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

feat(rest): endpoint to merge vendor. #2870

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rudra-superrr
Copy link
Contributor

@rudra-superrr rudra-superrr commented Jan 16, 2025

Closes: #2869

Description: this new endpoint will merge the source vendor to the target vendor.

@rudra-superrr rudra-superrr linked an issue Jan 16, 2025 that may be closed by this pull request
@rudra-superrr rudra-superrr added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for New-UI Level for the API and UI level changes for the new-ui labels Jan 16, 2025
@rudra-superrr rudra-superrr force-pushed the feat/mergeVendor branch 4 times, most recently from 685d793 to 8c8359b Compare January 28, 2025 07:00
@rudra-superrr
Copy link
Contributor Author

Made the changes.

@rudra-superrr
Copy link
Contributor Author

Made the changes.

@bibhuti230185
Copy link

bibhuti230185 commented Feb 12, 2025

The current implementation of the mergeVendors method in the VendorController class is not ideal because it mixes request parameters and request body in a PATCH request. A better approach would be to encapsulate all the required data in a single request body object. This makes the API cleaner and easier to use.
Here is a suggested approach:
Create a new class MergeVendorsRequest to encapsulate the request data.
Modify the mergeVendors method to accept this new class as the request body.

Test Findings:
Below 2 snarios to be looked up

  1. Merge fails if request body is not given. Is this ideal case? Or the request Body is required only if we need to modify the data other than the merged content
  2. Empty/null parameter throws 500 Internal error

Attached are the screenshot of API test through rest client

merge_fail_for_missing_param_value
merge_failed_for_requestbody_syntax_error
merge_failed_requestbody_mandatory
merge_success

@rudra-superrr
Copy link
Contributor Author

Hi @bibhuti230185 ,
Regarding the testing: yes, both the parameters and request body are mandatory in order to merge the vendor. In the second ss, it's showing error as the field(shortname) entered by you does not matches the field(shortName) present in the database.

In SW360, mixing request parameters and the request body in a PATCH request is generally acceptable. Since PATCH is intended for partial updates, it makes sense to send some parameters in the query string while including others in the request body. Additionally, we are not exposing any actual fields being updated—only the IDs, which is acceptable.

Copy link
Member

@GMishx GMishx left a comment

Choose a reason for hiding this comment

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

Minor documentation changes needed. Rest looks good.

content = {
@Content(mediaType = "application/json",
examples = @ExampleObject(
value = "{\"message\": \"Merge failed.\"}"
Copy link
Member

Choose a reason for hiding this comment

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

Please use actual message if possible.

Suggested change
value = "{\"message\": \"Merge failed.\"}"
value = "{\"message\": \"Vendor used as source or target has an open MR\"}"

Comment on lines 295 to 301
@ApiResponse(responseCode = "409", description = "Merge failed because vendor is used.",
content = {
@Content(mediaType = "application/json",
examples = @ExampleObject(
value = "{\"message\": \"Merge failed because vendor is used.\"}"
))
}),
Copy link
Member

Choose a reason for hiding this comment

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

409 conflict is not returned by this method as far as I can see. Instead there is a 500 undocumented with message Internal server error while merging the vendors.

Maybe you meant with the RequestStatus.IN_USE scenario which will currently throw 400 BAD REQUEST due to HttpMessageNotReadableException

@GMishx
Copy link
Member

GMishx commented Feb 19, 2025

The current implementation of the mergeVendors method in the VendorController class is not ideal because it mixes request parameters and request body in a PATCH request. A better approach would be to encapsulate all the required data in a single request body object. This makes the API cleaner and easier to use. Here is a suggested approach: Create a new class MergeVendorsRequest to encapsulate the request data. Modify the mergeVendors method to accept this new class as the request body.

The idea is good @bibhuti230185 and @rudra-superrr , however given the current situation of other endpoints the current implementation would suffice. We can work on simplifying the entire API when we have good documentation and the current state complete. And maybe even release v3 of the API with it.

@rudra-superrr
Copy link
Contributor Author

Hi @GMishx , made the changes as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review needs general test This is general testing, meaning that there is no org specific issue to check for New-UI Level for the API and UI level changes for the new-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New endpoint to merge vendor.
3 participants