Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OGUI-1604] Add middleware to 'edit layout' requests #2734

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

mariscalromeroalejandro
Copy link
Collaborator

  • branch and/or PR name(s) includes JIRA ID
  • issue has "Fix version" assigned
  • issue "Status" is set to "In review"
  • PR labels are selected
  • FLP integration tests were ran successful

This PR adds checks when editing layouts using PATCH and PUT requests:

  • Makes sure that Json File Service is provided.
  • Checks that the layout ID is included and actually exists in the system.
  • For PUT requests, ensures that only the owner can update their layout.
  • Verifies that the request contains a body.

@mariscalromeroalejandro mariscalromeroalejandro changed the title Feature/og UI 1604/add layout edit middleware [OGUI-1604] Add middleware to 'edit layout' requests Jan 29, 2025
Copy link
Member

@graduta graduta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mariscalromeroalejandro ,
The PR is in the right direction, good job!
Given that we move these checks to middleware function, and cleaning the tests for controller on such criteria, it means we are left exposed on API being tested against these scenarios.

Please have a look at the API tests from Control https://github.com/AliceO2Group/WebUi/tree/dev/Control/test/api and add some API tests for the PUT and PATCH layout:id paths as well

QualityControl/lib/middleware/requestBody.middleware.js Outdated Show resolved Hide resolved
Copy link
Member

@graduta graduta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction of the PullRequest is good. There are a few comments I have added plus, please add also API tests for PATCH

QualityControl/lib/controllers/LayoutController.js Outdated Show resolved Hide resolved
QualityControl/lib/controllers/LayoutController.js Outdated Show resolved Hide resolved
QualityControl/lib/controllers/LayoutController.js Outdated Show resolved Hide resolved
QualityControl/test/api/layouts/api-put-layout.test.js Outdated Show resolved Hide resolved
QualityControl/test/config.js Dismissed Show dismissed Hide dismissed
graduta
graduta previously approved these changes Jan 31, 2025
@graduta graduta self-requested a review January 31, 2025 14:33

test('should return a 404 error if the layout id is not provided', async () => {
await request(`${URL_ADDRESS}/api/layout/`)
.put(`?id=${null}&token=${OWNER_TEST_TOKEN}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the query parameter id used? As far as I can tell the id/name of the layout is taken from the req url param list

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The id and token are included in the URL as query parameters, not in the request body. They are still part of the query string in the URL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the API PUT route is not expecting a query parameter (the ones after ?)
It is expecting a URL parameters (the one after api/layout/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants