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

feat(rest): create new endpoint to delete ModerationRequests by id. #2363

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

feat(rest): create new endpoint to delete ModerationRequests by id. #2363

wants to merge 1 commit into from

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: Closes: #2362

Suggest Reviewer

You can suggest reviewers here with an @mention.

How To Test?

http://localhost:8080/resource/api/moderationrequest/delete/{id}

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

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from 96593e6 to ab23de1 Compare March 15, 2024 08:07
@ag4ums ag4ums added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for labels Mar 15, 2024
@rudra-superrr rudra-superrr self-assigned this Mar 21, 2024
@rudra-superrr
Copy link
Contributor

rest docs are broken.
image

@rudra-superrr
Copy link
Contributor

Give proper message when user is not a moderator/requesting user.
image

@rudra-superrr
Copy link
Contributor

Not able to delete MR when it is in approved state.
image

@rudra-superrr
Copy link
Contributor

As discussed on call -> instead of deleting a single MR, input list of ModerationRequest Ids that needs to be deleted from user.

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 2a26766 to cae0564 Compare March 26, 2024 07:19
@nikkuma7
Copy link
Contributor Author

@rudra-superrr, As per our discussion user can delete bulk moderation request based on Ids. And also deleting Approved MR as well.
image
image

@heliocastro heliocastro added the New-UI Level for the API and UI level changes for the new-ui label Mar 27, 2024
@rudra-superrr
Copy link
Contributor

Add an appropriate message and error code when user don't have write permission for MR.
image

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 4d0c45e to b1c519b Compare April 1, 2024 11:26
@rudra-superrr
Copy link
Contributor

According to UI, admins and moderators should not have permission to delete open moderation requests.
But your endpoint is allowing moderators to delete open MR.
image

@rudra-superrr
Copy link
Contributor

Status code should be 409 CONFLICT.
image

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 726bf96 to 980dd4b Compare April 10, 2024 07:12
@nikkuma7
Copy link
Contributor Author

Status code should be 409 CONFLICT. image

comment addressed.

@rudra-superrr
Copy link
Contributor

Change the status from Accepted to Not found if MR ids are incorrect or not present in db.
image

@rudra-superrr
Copy link
Contributor

Change status code from Accepted to Ok when MR is successfully deleted.
image

@rudra-superrr
Copy link
Contributor

Provide an appropriate message.
image

@rudra-superrr
Copy link
Contributor

rudra-superrr commented Apr 11, 2024 •
edited
Loading

Open MR cannot be deleted by any user other than the requesting user as discussed.
The functionality is fine i.e. MR is not getting deleted but response message is showing complete opposite.
image

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from 980dd4b to 881b2db Compare April 11, 2024 11:55
@nikkuma7
Copy link
Contributor Author

Open MR cannot be deleted by any user other than the requesting

comment addressed.

@rudra-superrr
Copy link
Contributor

image

image

Display a uniform message as in both the case MR cannot be deleted as it is in Open state and open MR can be deleted by requesting user only. Message can be like, MR is in open state and can be deleted by requesting user only.

@rudra-superrr
Copy link
Contributor

image

There are 2 MR in body, 225a31eacd8a4cc58358eaf1a4b3ee15 (open state) and 74708360c6c64372ec1900ae512dd93 (closed state) and user is a moderator.
It should be able to delete the MR which is in closed state but getting 403 status and not deleting the MR.

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 45e31e4 to 3608a1e Compare April 15, 2024 07:34
@nikkuma7
Copy link
Contributor Author

225a31eacd8a4cc58358eaf1a4b3ee15

Comment Addressed.

rudra-superrr
rudra-superrr requested changes Apr 16, 2024
View reviewed changes
...ava/org/eclipse/sw360/rest/resourceserver/moderationrequest/ModerationRequestController.java Outdated
}
}

public static boolean isOpenModerationRequest(@NotNull ModerationRequest moderationRequest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this method as it is already present in service class.

...ava/org/eclipse/sw360/rest/resourceserver/moderationrequest/ModerationRequestController.java
)
@PreAuthorize("hasAuthority('WRITE')")
@RequestMapping(value = MODERATION_REQUEST_URL + "/delete", method = RequestMethod.DELETE)
public ResponseEntity<?> deleteModerationRequest(HttpServletRequest request, @RequestBody List<String> ids,
Copy link
Contributor

Choose a reason for hiding this comment

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

use spaces for indentation instead of tabs.

...a/org/eclipse/sw360/rest/resourceserver/moderationrequest/Sw360ModerationRequestService.java Outdated
@@ -49,7 +54,7 @@ public class Sw360ModerationRequestService {
private String thriftServerUrl;

public static boolean isOpenModerationRequest(@NotNull ModerationRequest moderationRequest) {
return moderationRequest.getModerationState() == ModerationState.PENDING || moderationRequest.getModerationState() == ModerationState.INPROGRESS;
return moderationRequest.getModerationState() == ModerationState.PENDING || moderationRequest.getModerationState() == ModerationState.INPROGRESS || moderationRequest.getModerationState() == ModerationState.APPROVED;
Copy link
Contributor

Choose a reason for hiding this comment

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

why you added approved state in isOpenModerationRequest method? please remove it

...a/org/eclipse/sw360/rest/resourceserver/moderationrequest/Sw360ModerationRequestService.java Outdated
@@ -253,4 +265,38 @@ public ModerationState assignRequest(@NotNull ModerationRequest request, @NotNul
getThriftModerationClient().setInProgress(request.getId(), reviewer);
return ModerationState.INPROGRESS;
}

public RequestStatus deleteModerationRequestInfo(@NotNull User sw360User, @NotNull String id, @NotNull ModerationRequest moderationRequest, ModerationState moderationState)
Copy link
Contributor

Choose a reason for hiding this comment

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

use spaces for indentation instead of tabs.

rest/resource-server/src/docs/asciidoc/moderationRequests.adoc Outdated
A `DELETE` method will delete list of moderation request.

===== Example request
include::{snippets}/should_document_get_moderationrequests_delete/curl-request.adoc[]
Copy link
Contributor

Choose a reason for hiding this comment

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

change the method name to should_document_delete_moderationRequests

.../src/test/java/org/eclipse/sw360/rest/resourceserver/restdocs/ModerationRequestSpecTest.java Outdated
@@ -494,4 +501,33 @@ public void should_document_get_moderationrequests_submission() throws Exception
subsectionWithPath("_links").description("<<resources-index-links,Links>> to other resources")
)));
}

@Test
public void should_document_get_moderationrequests_delete() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

use spaces for indentation instead of tabs

@rudra-superrr
Copy link
Contributor

Document showing 500 internal server error.

image

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 789ca1d to 15a3606 Compare April 18, 2024 09:30
@rudra-superrr
Copy link
Contributor

Getting this in response,
image

message saying user is not a moderator but it a moderator. As MR is in open state, the message should be that MR can be deleted by requesting user only.

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from 15a3606 to d992913 Compare April 29, 2024 05:37
@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from d992913 to 0117a2b Compare May 17, 2024 05:44
@rudra-superrr
Copy link
Contributor

image

Out of the 2 MR ids, one got deleted. So please mention in message the MR id which cannot be deleted or is deleted.

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch 2 times, most recently from 7c3da97 to 6976709 Compare May 21, 2024 07:46
@GMishx
Copy link
Member

GMishx commented Sep 6, 2024

@nikkuma7 please rebase with latest main

@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from 6976709 to 75d06c3 Compare September 9, 2024 07:59
feat(rest): create new endpoint to delete ModerationRequests by id.
beee117 
Signed-off-by: Nikesh kumar <kumar.nikesh@simens.com>
@nikkuma7 nikkuma7 force-pushed the feat/DeleteModerationRequest-1480 branch from 75d06c3 to beee117 Compare September 10, 2024 05:06
@rudra-superrr
Copy link
Contributor

rudra-superrr commented Sep 13, 2024 •
edited
Loading

When an admin is deleting the MRs then the response is correct.
image

But when a requesting user is deleting the MRs then the response not showing the message for incorrect MR ids and the status is also showing 200 which should not be the case as some MR ids from the list are not present or incorrect. The status should be 409. Also give proper response message.
image

@GMishx GMishx assigned nikkuma7 and unassigned rudra-superrr Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudra-superrr rudra-superrr rudra-superrr requested changes

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

@GMishx GMishx Awaiting requested review from GMishx GMishx 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

@nikkuma7 nikkuma7

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(rest): create new endpoint to delete ModerationRequests by id.
5 participants
@nikkuma7 @rudra-superrr @GMishx @heliocastro @ag4ums