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.

Sign up for GitHub

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

Jump to bottom

fix(rest): Add license information linking for project releases. #2871

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

fix(rest): Add license information linking for project releases. #2871

wants to merge 1 commit into from
+196 −1

Conversation

nikkuma7
Copy link
Contributor

Please provide a summary of your changes here.

  • Which issue is this pull request belonging to and how is it solving it? (Refer to issue here)
  • Did you add or update any new dependencies that are required for your change?

Issue: #2844

Suggest Reviewer

You can suggest reviewers here with an @mention.

How To Test?

resource/api/projects/{id}/addLinkedRelesesLicenses
please check project should have licenses attached.

How should these changes be tested by the reviewer?
Have you implemented any additional tests?

Checklist

Must:

  • All related issues are referenced in commit messages and in PR

@GMishx GMishx added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for has merge conflicts The PR has merge conflicts labels Jan 27, 2025
GMishx
GMishx requested changes Jan 27, 2025
View reviewed changes
...-server/src/main/java/org/eclipse/sw360/rest/resourceserver/project/Sw360ProjectService.java Outdated
jsonResult.put(SW360Constants.STATUS, SW360Constants.FAILURE);
}

componentClient.updateRelease(release, sw360User);
Copy link
Member

Choose a reason for hiding this comment

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

This call can result in creation of Moderation Request. Please add comment field for the same in the controller as done in #2405

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GMishx , For this endpoint, a Moderation Request is not required, so adding a comment field is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

@nikkuma7 , I get it now, the function processSingleAttachment() updates the release by calling setMainLicenseIds() and setOtherLicenseIds().

Thus, calling componentClient.updateRelease() can result in moderation request. The comparator for Release does checks the mainLicenseIds and otherLicenseIds fields.

Please test again!

Copy link
Member

Choose a reason for hiding this comment

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

@nikkuma7 any updates on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A moderation request is now created when clicking the "Add License Info" button. The moderation request is also created at the time of attachment creation or when modifying the attachment file. However, the moderation request is not created when clicking the "License Info" button.

@GMishx
Copy link
Member

GMishx commented Jan 31, 2025

Any updates @nikkuma7 ?

@nikkuma7 nikkuma7 force-pushed the feat/Add_License_Info_to_Release branch from de1b3a4 to 42d0655 Compare February 4, 2025 04:42
@nikkuma7
Copy link
Contributor Author

nikkuma7 commented Feb 4, 2025

Any updates @nikkuma7 ?

comment addressed.

@nikkuma7 nikkuma7 removed the has merge conflicts The PR has merge conflicts label Feb 4, 2025
@rudra-superrr rudra-superrr added the has merge conflicts The PR has merge conflicts label Feb 6, 2025
@nikkuma7 nikkuma7 force-pushed the feat/Add_License_Info_to_Release branch from 42d0655 to fc1d47d Compare February 7, 2025 06:18
@nikkuma7 nikkuma7 removed the has merge conflicts The PR has merge conflicts label Feb 7, 2025
@GMishx
Copy link
Member

GMishx commented Feb 19, 2025

@nikkuma7 looks like the above 2 comments from me are not incorporated. Please have a look again.

@nikkuma7 nikkuma7 force-pushed the feat/Add_License_Info_to_Release branch from fc1d47d to dd6a077 Compare March 3, 2025 04:46
GMishx
GMishx requested changes Mar 3, 2025
View reviewed changes
...-server/src/main/java/org/eclipse/sw360/rest/resourceserver/project/Sw360ProjectService.java Outdated
jsonResult.put(SW360Constants.STATUS, SW360Constants.FAILURE);
}

componentClient.updateRelease(release, sw360User);
Copy link
Member

Choose a reason for hiding this comment

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

@nikkuma7 , I get it now, the function processSingleAttachment() updates the release by calling setMainLicenseIds() and setOtherLicenseIds().

Thus, calling componentClient.updateRelease() can result in moderation request. The comparator for Release does checks the mainLicenseIds and otherLicenseIds fields.

Please test again!

@nikkuma7 nikkuma7 force-pushed the feat/Add_License_Info_to_Release branch from dd6a077 to 4bdbad8 Compare March 5, 2025 05:34
@GMishx GMishx mentioned this pull request Mar 6, 2025
Open
GMishx
GMishx requested changes Mar 11, 2025
View reviewed changes
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.

@nikkuma7 can you please take a look and close this PR ASAP? UI will be needing it soon.

...-server/src/main/java/org/eclipse/sw360/rest/resourceserver/project/Sw360ProjectService.java Outdated
Comment on lines 1652 to 1656
log.info("Updating licenses for release: {}", release.getId());
release.setMainLicenseIds(mainLicenses);
release.setOtherLicenseIds(otherLicenses);
} else {
log.info("No changes detected for release: {}", release.getId());
Copy link
Member

Choose a reason for hiding this comment

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

The logs here are more of debug statement IMO. I'd use log.debug instead of log.info

...rce-server/src/test/java/org/eclipse/sw360/rest/resourceserver/restdocs/ProjectSpecTest.java Outdated

MockHttpServletRequestBuilder requestBuilder = post("/api/projects/" + projectId + "/addLinkedRelesesLicenses")
.contentType(MediaType.APPLICATION_JSON)
.param("comment", comment)
Copy link
Member

Choose a reason for hiding this comment

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

The param comment is sent here but not read in the controller. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@nikkuma7 nikkuma7 force-pushed the feat/Add_License_Info_to_Release branch from 4bdbad8 to 1ab8200 Compare March 11, 2025 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GMishx GMishx GMishx requested changes

@heliocastro heliocastro Awaiting requested review from heliocastro heliocastro is a code owner

@KoukiHama KoukiHama Awaiting requested review from KoukiHama KoukiHama is a code owner

@arunazhakesan arunazhakesan Awaiting requested review from arunazhakesan arunazhakesan is a code owner

@ag4ums ag4ums Awaiting requested review from ag4ums ag4ums is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs code review needs general test This is general testing, meaning that there is no org specific issue to check for
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nikkuma7 @GMishx @rudra-superrr