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(api): Update Upload Summary API to add more info #2499

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

dushimsam
Copy link
Contributor

@dushimsam dushimsam commented Jun 26, 2023

Description

Update the current API to for getting the upload's summary to add more information related to licenses.

Changes

  1. Updated the method getUploadSummary in UploadController to make the changes.
  2. Updated the openapi.yaml file to introduce a new changes for the API.

How to test

Make a GET request on the endpoint: /uploads/{id}/summary.

Screenshots

image

Related Issue:

Fixes #2467

cc: @shaheemazmalmmd @GMishx

@GMishx GMishx added needs code review needs test GSOC-23 Label to tag pull request which are part of the GSOC 2023 labels Jun 28, 2023
@github-actions github-actions bot added the has merge conflicts PR to be rebased label Jun 29, 2023
@github-actions
Copy link

This pull request has conflicts, please rebase with master to resolve those before we can evaluate the pull request.

@GMishx
Copy link
Member

GMishx commented Jul 10, 2023

Would recommend extending existing endpoint /uploads/{id}/summary as it already contains most of the required information.

Existing endpoint of left, new endpoint on the right.
image

@dushimsam
Copy link
Contributor Author

Would recommend extending existing endpoint /uploads/{id}/summary as it already contains most of the required information.

Existing endpoint of left, new endpoint on the right. image

Yes, that looks better.

@dushimsam
Copy link
Contributor Author

dushimsam commented Jul 10, 2023

But again @GMishx, The new API only returns the summary of upload for the licenses section, not for other modules like copyrights. The values returned by the new API also vary depending on the agentId selected. Given these factors, do you think it is appropriate to include them in the existing implementation? Let me know what you think and then I will go with your suggestion. Thank you.

@GMishx
Copy link
Member

GMishx commented Jul 11, 2023

Oh, I didn't notice the endpoint is dependent on agent id. But still, they can be merged if the agent id is not provided, the current behavior can be followed. Otherwise, the values will change based on agent id.

It is better to send little more information from a single endpoint than creating one more which shares the same info. Creating new endpoint will also cause confusion if more than 1 endpoint can be used to get same info (and will also need multiple calls if info from both endpoint is needed :-))

@github-actions github-actions bot removed the has merge conflicts PR to be rebased label Jul 11, 2023
@dushimsam dushimsam changed the title feat(api): Get licenses count summary API feat(api): Update Upload Summary API to add more additional info Jul 11, 2023
@dushimsam
Copy link
Contributor Author

Awesome, I have applied the new changes to the current implementation and have revised the commit and the PR's description.
Please let me know if there is anything else I need to modify.

Thank you very much.

Signed-off-by: dushimsam <dushsam@gmail.com>
@dushimsam dushimsam changed the title feat(api): Update Upload Summary API to add more additional info feat(api): Update Upload Summary API to add more info Jul 11, 2023
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.

Changes looks good.
Tested, working as expected.

@GMishx GMishx merged commit a433bd9 into fossology:master Jul 24, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSOC-23 Label to tag pull request which are part of the GSOC 2023 ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

License Browser - Scanner Licenses
2 participants