SONAR-16147 Allow users to assign acknowledged Security Hotspots

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<Props, State> {
   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 (
       <AssigneeRenderer

server/sonar-web/src/main/js/apps/security-hotspots/components/assignee/__tests__/Assignee-test.tsx

@@ -24,7 +24,7 @@ import addGlobalSuccessMessage from '../../../../../app/utils/addGlobalSuccessMe
 import { mockHotspot } from '../../../../../helpers/mocks/security-hotspots';
 import { mockCurrentUser, mockUser } from '../../../../../helpers/testMocks';
 import { waitAndUpdate } from '../../../../../helpers/testUtils';
-import { HotspotStatus } from '../../../../../types/security-hotspots';
+import { HotspotResolution, HotspotStatus } from '../../../../../types/security-hotspots';
 import { UserActive } from '../../../../../types/users';
 import { Assignee } from '../Assignee';
 import AssigneeRenderer from '../AssigneeRenderer';
@@ -37,9 +37,19 @@ jest.mock('../../../../../app/utils/addGlobalSuccessMessage', () => 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', () => {

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`] = `
-<AssigneeRenderer
-  assignee={
-    Object {
-      "active": true,
-      "local": true,
-      "login": "assignee",
-      "name": "John Doe",
-    }
-  }
-  canEdit={true}
-  editing={false}
-  loading={false}
-  onAssign={[Function]}
-  onEnterEditionMode={[Function]}
-  onExitEditionMode={[Function]}
-/>
-`;

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");
     }
   }
 

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