From 8ecc599d9bcbee170569b2f11d1ea583f1d2354d Mon Sep 17 00:00:00 2001 From: Philippe Perrin Date: Tue, 22 Mar 2022 14:03:04 +0100 Subject: [PATCH] SONAR-16147 Allow users to assign acknowledged Security Hotspots --- .../components/assignee/Assignee.tsx | 7 +-- .../assignee/__tests__/Assignee-test.tsx | 16 ++++-- .../__snapshots__/Assignee-test.tsx.snap | 19 ------- .../sonar/server/hotspot/ws/AssignAction.java | 11 ++-- .../server/hotspot/ws/AssignActionTest.java | 53 +++++++++++++------ 5 files changed, 59 insertions(+), 47 deletions(-) diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/Assignee.tsx b/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/Assignee.tsx index 89a43531b3b..51f34af34bc 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/Assignee.tsx +++ b/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/Assignee.tsx @@ -22,7 +22,7 @@ import { assignSecurityHotspot } from '../../../../api/security-hotspots'; import withCurrentUserContext from '../../../../app/components/current-user/withCurrentUserContext'; import addGlobalSuccessMessage from '../../../../app/utils/addGlobalSuccessMessage'; import { translate, translateWithParameters } from '../../../../helpers/l10n'; -import { Hotspot, HotspotStatus } from '../../../../types/security-hotspots'; +import { Hotspot, HotspotResolution, HotspotStatus } from '../../../../types/security-hotspots'; import { CurrentUser, isLoggedIn, UserActive } from '../../../../types/users'; import AssigneeRenderer from './AssigneeRenderer'; @@ -85,11 +85,12 @@ export class Assignee extends React.PureComponent { render() { const { currentUser, - hotspot: { assigneeUser, status } + hotspot: { assigneeUser, status, resolution } } = this.props; const { editing, loading } = this.state; - const canEdit = status === HotspotStatus.TO_REVIEW; + const canEdit = + status === HotspotStatus.TO_REVIEW || resolution === HotspotResolution.ACKNOWLEDGED; return ( jest.fn()); it('should render correctly', () => { expect(shallowRender()).toMatchSnapshot(); +}); + +it.each([ + [HotspotStatus.TO_REVIEW, undefined, true], + [HotspotStatus.REVIEWED, HotspotResolution.FIXED, false], + [HotspotStatus.REVIEWED, HotspotResolution.SAFE, false], + [HotspotStatus.REVIEWED, HotspotResolution.ACKNOWLEDGED, true] +])('should allow edition properly', (status, resolution, canEdit) => { expect( - shallowRender({ hotspot: mockHotspot({ status: HotspotStatus.TO_REVIEW }) }) - ).toMatchSnapshot('can edit'); + shallowRender({ hotspot: mockHotspot({ status, resolution }) }) + .find(AssigneeRenderer) + .props().canEdit + ).toBe(canEdit); }); it('should handle edition event correctly', () => { diff --git a/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/__tests__/__snapshots__/Assignee-test.tsx.snap b/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/__tests__/__snapshots__/Assignee-test.tsx.snap index 1cb486f7a56..1b919702f03 100644 --- a/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/__tests__/__snapshots__/Assignee-test.tsx.snap +++ b/server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/__tests__/__snapshots__/Assignee-test.tsx.snap @@ -18,22 +18,3 @@ exports[`should render correctly 1`] = ` onExitEditionMode={[Function]} /> `; - -exports[`should render correctly: can edit 1`] = ` - -`; diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/AssignAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/AssignAction.java index 7659b83ed5b..f04a4d29ad4 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/AssignAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/hotspot/ws/AssignAction.java @@ -37,6 +37,7 @@ import org.sonar.server.issue.IssueFieldsSetter; import org.sonar.server.issue.ws.IssueUpdater; import static com.google.common.base.Strings.isNullOrEmpty; +import static org.sonar.api.issue.Issue.RESOLUTION_ACKNOWLEDGED; import static org.sonar.api.issue.Issue.STATUS_TO_REVIEW; import static org.sonar.server.exceptions.NotFoundException.checkFound; import static org.sonar.server.exceptions.NotFoundException.checkFoundWithOptional; @@ -102,9 +103,9 @@ public class AssignAction implements HotspotsWsAction { IssueDto hotspotDto = hotspotWsSupport.loadHotspot(dbSession, hotspotKey); - checkIfHotspotToReview(hotspotDto); + checkHotspotStatusAndResolution(hotspotDto); hotspotWsSupport.loadAndCheckProject(dbSession, hotspotDto, UserRole.USER); - UserDto assignee = isNullOrEmpty(login) ? null :getAssignee(dbSession, login); + UserDto assignee = isNullOrEmpty(login) ? null : getAssignee(dbSession, login); IssueChangeContext context = hotspotWsSupport.newIssueChangeContext(); @@ -124,9 +125,9 @@ public class AssignAction implements HotspotsWsAction { } } - private static void checkIfHotspotToReview(IssueDto hotspotDto) { - if (!STATUS_TO_REVIEW.equals(hotspotDto.getStatus())) { - throw new IllegalArgumentException(String.format("Assignee can only be changed on Security Hotspots with status '%s'", STATUS_TO_REVIEW)); + private static void checkHotspotStatusAndResolution(IssueDto hotspotDto) { + if (!STATUS_TO_REVIEW.equals(hotspotDto.getStatus()) && !RESOLUTION_ACKNOWLEDGED.equals(hotspotDto.getResolution())) { + throw new IllegalArgumentException("Cannot change the assignee of this hotspot given its current status and resolution"); } } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/AssignActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/AssignActionTest.java index 70561138831..7d80092e4fe 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/AssignActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/hotspot/ws/AssignActionTest.java @@ -23,6 +23,7 @@ import com.google.common.collect.Sets; import com.tngtech.java.junit.dataprovider.DataProvider; import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; +import java.util.Arrays; import java.util.EnumSet; import java.util.List; import java.util.Set; @@ -58,6 +59,7 @@ import org.sonar.server.ws.WsActionTester; import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatNoException; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.tuple; import static org.mockito.ArgumentMatchers.any; @@ -68,8 +70,12 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import static org.sonar.api.issue.Issue.RESOLUTION_ACKNOWLEDGED; +import static org.sonar.api.issue.Issue.RESOLUTION_FIXED; +import static org.sonar.api.issue.Issue.RESOLUTION_SAFE; import static org.sonar.api.issue.Issue.STATUSES; import static org.sonar.api.issue.Issue.STATUS_CLOSED; +import static org.sonar.api.issue.Issue.STATUS_REVIEWED; import static org.sonar.api.issue.Issue.STATUS_TO_REVIEW; import static org.sonar.api.rules.RuleType.SECURITY_HOTSPOT; import static org.sonar.db.component.ComponentTesting.newFileDto; @@ -334,12 +340,15 @@ public class AssignActionTest { } @Test - @UseDataProvider("allIssueStatusesExceptToReviewAndClosed") - public void fail_if_assign_user_to_hotspot_for_OTHER_STATUSES_for_public_project(String status) { + @UseDataProvider("allIssueStatusesAndResolutionsThatThrowOnAssign") + public void fail_if_assign_user_to_hotspot_for_which_it_is_forbidden(String status, String resolution) { ComponentDto project = dbTester.components().insertPublicProject(); ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); - IssueDto hotspot = dbTester.issues().insertHotspot(project, file, h -> h.setStatus(status)); + IssueDto hotspot = dbTester.issues().insertHotspot(project, file, h -> { + h.setStatus(status); + h.setResolution(resolution); + }); UserDto userDto = insertUser(randomAlphanumeric(10)); userSessionRule.logIn(userDto).registerComponents(project); @@ -347,32 +356,42 @@ public class AssignActionTest { String login = userSessionRule.getLogin(); assertThatThrownBy(() -> executeRequest(hotspot, login, null)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Assignee can only be changed on Security Hotspots with status 'TO_REVIEW'"); + .hasMessage("Cannot change the assignee of this hotspot given its current status and resolution"); + } + + @DataProvider + public static Object[][] allIssueStatusesAndResolutionsThatThrowOnAssign() { + return STATUSES.stream() + .filter(status -> !STATUS_TO_REVIEW.equals(status)) + .filter(status -> !STATUS_CLOSED.equals(status)) + .flatMap(status -> Arrays.stream(new Object[] {RESOLUTION_SAFE, RESOLUTION_FIXED}) + .map(resolution -> new Object[] {status, resolution})) + .toArray(Object[][]::new); } @Test - @UseDataProvider("allIssueStatusesExceptToReviewAndClosed") - public void fail_if_assign_user_to_hotspot_for_OTHER_STATUSES_for_private_project(String status) { - ComponentDto project = dbTester.components().insertPrivateProject(); + @UseDataProvider("allIssueStatusesAndResolutionsThatDoNotThrowOnAssign") + public void fail_if_assign_user_to_hotspot_for_which_it_is_allowed(String status, String resolution) { + ComponentDto project = dbTester.components().insertPublicProject(); ComponentDto file = dbTester.components().insertComponent(newFileDto(project)); - IssueDto hotspot = dbTester.issues().insertHotspot(project, file, h -> h.setStatus(status)); + IssueDto hotspot = dbTester.issues().insertHotspot(project, file, h -> { + h.setStatus(status); + h.setResolution(resolution); + }); UserDto userDto = insertUser(randomAlphanumeric(10)); userSessionRule.logIn(userDto).registerComponents(project); String login = userSessionRule.getLogin(); - assertThatThrownBy(() -> executeRequest(hotspot, login, null)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Assignee can only be changed on Security Hotspots with status 'TO_REVIEW'"); + assertThatNoException().isThrownBy(() -> executeRequest(hotspot, login, null)); } @DataProvider - public static Object[][] allIssueStatusesExceptToReviewAndClosed() { - return STATUSES.stream() - .filter(status -> !STATUS_TO_REVIEW.equals(status)) - .filter(status -> !STATUS_CLOSED.equals(status)) - .map(status -> new Object[] {status}) - .toArray(Object[][]::new); + public static Object[][] allIssueStatusesAndResolutionsThatDoNotThrowOnAssign() { + return new Object[][] { + new Object[] {STATUS_TO_REVIEW, null}, + new Object[] {STATUS_REVIEWED, RESOLUTION_ACKNOWLEDGED} + }; } @Test -- 2.39.5