Enable the delete workitem endpoint (#2305)

Enables delete workitem endpoint

controller/workitem.go

@@ -71,23 +71,21 @@ func NewNotifyingWorkitemController(service *goa.Service, db application.DB, not
 		config:       config}
 }
 
-// authorizeWorkitemTypeEditor returns true if the modifier is allowed to change
-// workitem type else it returns false.
-// Only space owner and workitem creator are allowed to change workitem type
-func (c *WorkitemController) authorizeWorkitemTypeEditor(ctx context.Context, spaceID uuid.UUID, creatorID string, editorID string) (bool, error) {
+// WorkitemCreatorOrSpaceOwner checks if the modifier is space owner or workitem creator
+func (c *WorkitemController) WorkitemCreatorOrSpaceOwner(ctx context.Context, spaceID uuid.UUID, creatorID uuid.UUID, editorID uuid.UUID) error {
 	// check if workitem editor is same as workitem creator
 	if editorID == creatorID {
-		return true, nil
+		return nil
 	}
 	space, err := c.db.Spaces().Load(ctx, spaceID)
 	if err != nil {
-		return false, errors.NewNotFoundError("space", spaceID.String())
+		return errors.NewNotFoundError("space", spaceID.String())
 	}
 	// check if workitem editor is same as space owner
-	if space != nil && editorID == space.OwnerID.String() {
-		return true, nil
+	if space != nil && editorID == space.OwnerID {
+		return nil
 	}
-	return false, errors.NewUnauthorizedError("user is not allowed to change workitem type")
+	return errors.NewForbiddenError("user is not a workitem creator or space owner")
 }
 
 // Returns true if the user is the work item creator or space collaborator
@@ -123,6 +121,14 @@ func (c *WorkitemController) Update(ctx *app.UpdateWorkitemContext) error {
 	if creator == nil {
 		return jsonapi.JSONErrorResponse(ctx, errors.NewInternalError(ctx, errs.New("work item doesn't have creator")))
 	}
+	creatorIDStr, ok := creator.(string)
+	if !ok {
+		return jsonapi.JSONErrorResponse(ctx, errs.Errorf("failed to convert user to string: %+v (%[1]T)", creator))
+	}
+	creatorID, err := uuid.FromString(creatorIDStr)
+	if err != nil {
+		return jsonapi.JSONErrorResponse(ctx, err)
+	}
 	authorized, err := authorizeWorkitemEditor(ctx, c.db, wi.SpaceID, creator.(string), currentUserIdentityID.String())
 	if err != nil {
 		return jsonapi.JSONErrorResponse(ctx, err)
@@ -134,13 +140,10 @@ func (c *WorkitemController) Update(ctx *app.UpdateWorkitemContext) error {
 	if ctx.Payload.Data.Relationships != nil && ctx.Payload.Data.Relationships.BaseType != nil &&
 		ctx.Payload.Data.Relationships.BaseType.Data != nil && ctx.Payload.Data.Relationships.BaseType.Data.ID != wi.Type {
 
-		authorized, err := c.authorizeWorkitemTypeEditor(ctx, wi.SpaceID, creator.(string), currentUserIdentityID.String())
+		err := c.WorkitemCreatorOrSpaceOwner(ctx, wi.SpaceID, creatorID, *currentUserIdentityID)
 		if err != nil {
 			return jsonapi.JSONErrorResponse(ctx, err)
 		}
-		if !authorized {
-			return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to change the workitemtype"))
-		}
 		// Store new values of type and version
 		newType := ctx.Payload.Data.Relationships.BaseType
 		newVersion := ctx.Payload.Data.Attributes[workitem.SystemVersion]
@@ -236,39 +239,52 @@ func (c *WorkitemController) Show(ctx *app.ShowWorkitemContext) error {
 
 // Delete does DELETE workitem
 func (c *WorkitemController) Delete(ctx *app.DeleteWorkitemContext) error {
-	// Temporarly disabled, See https://github.com/fabric8-services/fabric8-wit/issues/1036
-	if true {
-		return ctx.MethodNotAllowed()
-	}
 	currentUserIdentityID, err := login.ContextIdentity(ctx)
 	if err != nil {
 		return jsonapi.JSONErrorResponse(ctx, errors.NewUnauthorizedError(err.Error()))
 	}
+
 	var wi *workitem.WorkItem
 	err = application.Transactional(c.db, func(appl application.Application) error {
 		wi, err = appl.WorkItems().LoadByID(ctx, ctx.WiID)
 		if err != nil {
-			return errs.Wrap(err, fmt.Sprintf("Failed to load work item with id %v", ctx.WiID))
+			return errs.Wrap(err, fmt.Sprintf("Fail to load work item with id %v", ctx.WiID))
 		}
 		return nil
 	})
 	if err != nil {
 		return jsonapi.JSONErrorResponse(ctx, err)
 	}
-	authorized, err := authz.Authorize(ctx, wi.SpaceID.String())
+
+	// Check if user is space owner or workitem creator. Only space owner or workitem creator are allowed to delete the workitem.
+	creator := wi.Fields[workitem.SystemCreator]
+	if creator == nil {
+		return jsonapi.JSONErrorResponse(ctx, errors.NewInternalError(ctx, errs.New("work item doesn't have creator")))
+	}
+	creatorIDStr, ok := creator.(string)
+	if !ok {
+		return jsonapi.JSONErrorResponse(ctx, errs.Errorf("failed to convert user to string: %+v (%[1]T)", creator))
+	}
+	creatorID, err := uuid.FromString(creatorIDStr)
 	if err != nil {
-		return jsonapi.JSONErrorResponse(ctx, errors.NewUnauthorizedError(err.Error()))
+		return jsonapi.JSONErrorResponse(ctx, err)
 	}
-	if !authorized {
-		return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to access the space"))
+	err = c.WorkitemCreatorOrSpaceOwner(ctx, wi.SpaceID, creatorID, *currentUserIdentityID)
+	if err != nil {
+		forbidden, _ := errors.IsForbiddenError(err)
+		if forbidden {
+			return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to delete the workitem"))
+
+		}
+		return jsonapi.JSONErrorResponse(ctx, err)
 	}
 	err = application.Transactional(c.db, func(appl application.Application) error {
-		if err := appl.WorkItems().Delete(ctx, ctx.WiID, *currentUserIdentityID); err != nil {
-			return errs.Wrapf(err, "error deleting work item %s", ctx.WiID)
-		}
 		if err := appl.WorkItemLinks().DeleteRelatedLinks(ctx, ctx.WiID, *currentUserIdentityID); err != nil {
 			return errs.Wrapf(err, "failed to delete work item links related to work item %s", ctx.WiID)
 		}
+		if err := appl.WorkItems().Delete(ctx, ctx.WiID, *currentUserIdentityID); err != nil {
+			return errs.Wrapf(err, "error deleting work item %s", ctx.WiID)
+		}
 		return nil
 	})
 	if err != nil {

controller/workitem_blackbox_test.go

@@ -2304,17 +2304,6 @@ func (s *WorkItem2Suite) TestWI2FailShowMissing() {
 	test.ShowWorkitemNotFound(s.T(), s.svc.Context, s.svc, s.workitemCtrl, uuid.NewV4(), nil, nil)
 }
 
-func (s *WorkItem2Suite) TestWI2FailOnDelete() {
-	c := minimumRequiredCreatePayload()
-	c.Data.Attributes[workitem.SystemTitle] = "Title"
-	c.Data.Attributes[workitem.SystemState] = workitem.SystemStateNew
-	c.Data.Relationships.BaseType = newRelationBaseType(workitem.SystemBug)
-
-	_, createdWI := test.CreateWorkitemsCreated(s.T(), s.svc.Context, s.svc, s.workitemsCtrl, *c.Data.Relationships.Space.Data.ID, &c)
-	test.ShowWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, *createdWI.Data.ID, nil, nil)
-	test.DeleteWorkitemMethodNotAllowed(s.T(), s.svc.Context, s.svc, s.workitemCtrl, *createdWI.Data.ID)
-}
-
 func (s *WorkItem2Suite) TestWI2CreateWithArea() {
 	fxt := tf.NewTestFixture(s.T(), s.DB,
 		tf.CreateWorkItemEnvironment(),
@@ -3258,8 +3247,7 @@ func (s *WorkItemSuite) TestUpdateWorkitemForSpaceCollaborator() {
 	// Not a space collaborator is not authorized to update
 	test.UpdateWorkitemForbidden(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID, &payload2)
 	// Not a space collaborator is not authorized to delete
-	// Temporarily disabled, See https://github.com/fabric8-services/fabric8-wit/issues/1036
-	// test.DeleteWorkitemForbidden(s.T(), svcNotAuthrized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID)
+	test.DeleteWorkitemForbidden(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, *wi.Data.ID)
 	// Not a space collaborator is not authorized to reorder
 	payload5 := minimumRequiredReorderPayload()
 	var dataArray []*app.WorkItem // dataArray contains the workitem(s) that have to be reordered
@@ -3390,3 +3378,43 @@ func (s *WorkItem2Suite) TestCreateAndUpdateWorkItemForEveryWIT() {
 		})
 	}
 }
+
+// TestDeleteWorkitem tests the delete action
+func (s *WorkItem2Suite) TestDeleteWorkitem() {
+	// Delete Workitem tests deletion of workitem
+	s.T().Run("Delete Workitem", func(t *testing.T) {
+		t.Run("ok", func(t *testing.T) {
+			fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1))
+			s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[0])
+			test.DeleteWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID)
+		})
+		t.Run("unauthorized", func(t *testing.T) {
+			fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1))
+			svcNotAuthorized := goa.New("TestDeleteWI2-Service")
+			workitemCtrlNotAuthorized := NewWorkitemController(svcNotAuthorized, s.GormDB, s.Configuration)
+			test.DeleteWorkitemUnauthorized(s.T(), svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, fxt.WorkItems[0].ID)
+		})
+		t.Run("forbidden", func(t *testing.T) {
+			fxt := tf.NewTestFixture(s.T(), s.DB, tf.WorkItems(1), tf.Identities(2))
+			s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[1])
+			test.DeleteWorkitemForbidden(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID)
+		})
+		t.Run("workitem not found", func(t *testing.T) {
+			test.DeleteWorkitemNotFound(s.T(), s.svc.Context, s.svc, s.workitemCtrl, uuid.NewV4())
+		})
+	})
+	// Delete Workitem Links tests deletion of corresponding workitem links when a workitem is deleted
+	s.T().Run("Delete Workitem Links", func(t *testing.T) {
+		t.Run("ok", func(t *testing.T) {
+			fxt := tf.NewTestFixture(t, s.DB,
+				tf.CreateWorkItemEnvironment(),
+				tf.WorkItems(2, tf.SetWorkItemTitles("A", "B")),
+				tf.WorkItemLinkTypes(1),
+				tf.WorkItemLinksCustom(1, tf.BuildLinks(tf.LinkChain("A", "B")...)),
+			)
+			s.svc = testsupport.ServiceAsUser("TestUpdateWI2-Service", *fxt.Identities[0])
+			test.DeleteWorkitemOK(s.T(), s.svc.Context, s.svc, s.workitemCtrl, fxt.WorkItems[0].ID)
+			test.ShowWorkItemLinkNotFound(t, s.svc.Context, s.svc, s.linkCtrl, fxt.WorkItemLinks[0].ID, nil, nil)
+		})
+	})
+}

design/workitems.go

@@ -152,9 +152,7 @@ var _ = a.Resource("workitem", func() {
 		a.Params(func() {
 			a.Param("wiID", d.UUID, "ID of the work item to delete")
 		})
-		a.Response(d.MethodNotAllowed)
 		a.Response(d.OK)
-		a.Response(d.BadRequest, JSONAPIErrors)
 		a.Response(d.InternalServerError, JSONAPIErrors)
 		a.Response(d.NotFound, JSONAPIErrors)
 		a.Response(d.Unauthorized, JSONAPIErrors)

workitem/link/link_repository_blackbox_test.go

@@ -892,3 +892,16 @@ func (s *linkRepoBlackBoxTest) TestDeleteLinkAndListChildren() {
 		require.Len(t, childrenList, 0)
 	})
 }
+
+func (s *linkRepoBlackBoxTest) TestDeleteLink() {
+	s.T().Run("ok", func(t *testing.T) {
+		fxt := tf.NewTestFixture(t, s.DB, tf.WorkItemLinks(1))
+		err := s.workitemLinkRepo.DeleteRelatedLinks(s.Ctx, fxt.WorkItems[0].ID, fxt.Identities[0].ID)
+		require.NoError(t, err)
+
+		// check if link exists
+		err = s.workitemLinkRepo.CheckExists(s.Ctx, fxt.WorkItemLinks[0].ID)
+		require.Error(t, err)
+		require.IsType(t, errors.NotFoundError{}, err)
+	})
+}

workitem/workitem_repository_blackbox_test.go

@@ -769,3 +769,18 @@ func (s *workItemRepoBlackBoxTest) TestList() {
 
 	})
 }
+
+func (s *workItemRepoBlackBoxTest) TestDeleteWorkitem() {
+	s.T().Run("ok", func(t *testing.T) {
+		fxt := tf.NewTestFixture(t, s.DB,
+			tf.WorkItems(1),
+		)
+		err := s.repo.Delete(s.Ctx, fxt.WorkItems[0].ID, fxt.Identities[0].ID)
+		require.Nil(t, err)
+
+		// check if workitem exists
+		err = s.repo.CheckExists(s.Ctx, fxt.WorkItems[0].ID)
+		require.Error(t, err)
+		require.IsType(t, errors.NotFoundError{}, errs.Cause(err))
+	})
+}