refactor(projects): dedupe effective parent lookup, tidy error ordering and comments

Also adds a negative control to the structure-import archive test.

pkg/models/error.go

@@ -364,30 +364,6 @@ func (err ErrProjectIsArchived) HTTPError() web.HTTPError {
 	return web.HTTPError{HTTPCode: http.StatusPreconditionFailed, Code: ErrCodeProjectIsArchived, Message: "This project is archived. Editing or creating new tasks is not possible."}
 }
 
-// ErrParentProjectIsArchived represents an error, where a project's parent is archived
-type ErrParentProjectIsArchived struct {
-	ProjectID       int64
-	ParentProjectID int64
-}
-
-// IsErrParentProjectIsArchived checks if an error is a parent project is archived error.
-func IsErrParentProjectIsArchived(err error) bool {
-	_, ok := err.(ErrParentProjectIsArchived)
-	return ok
-}
-
-func (err ErrParentProjectIsArchived) Error() string {
-	return fmt.Sprintf("Parent project is archived [ProjectID: %d, ParentProjectID: %d]", err.ProjectID, err.ParentProjectID)
-}
-
-// ErrCodeParentProjectIsArchived holds the unique world-error code of this error
-const ErrCodeParentProjectIsArchived = 3016
-
-// HTTPError holds the http error description
-func (err ErrParentProjectIsArchived) HTTPError() web.HTTPError {
-	return web.HTTPError{HTTPCode: http.StatusPreconditionFailed, Code: ErrCodeParentProjectIsArchived, Message: "The parent project is archived. Un-archive the parent project first."}
-}
-
 // ErrProjectCannotBelongToAPseudoParentProject represents an error where a project cannot belong to a pseudo project
 type ErrProjectCannotBelongToAPseudoParentProject struct {
 	ProjectID       int64
@@ -588,6 +564,30 @@ func (err *ErrProjectHasNoBackground) HTTPError() web.HTTPError {
 	}
 }
 
+// ErrParentProjectIsArchived represents an error, where a project's parent is archived
+type ErrParentProjectIsArchived struct {
+	ProjectID       int64
+	ParentProjectID int64
+}
+
+// IsErrParentProjectIsArchived checks if an error is a parent project is archived error.
+func IsErrParentProjectIsArchived(err error) bool {
+	_, ok := err.(ErrParentProjectIsArchived)
+	return ok
+}
+
+func (err ErrParentProjectIsArchived) Error() string {
+	return fmt.Sprintf("Parent project is archived [ProjectID: %d, ParentProjectID: %d]", err.ProjectID, err.ParentProjectID)
+}
+
+// ErrCodeParentProjectIsArchived holds the unique world-error code of this error
+const ErrCodeParentProjectIsArchived = 3016
+
+// HTTPError holds the http error description
+func (err ErrParentProjectIsArchived) HTTPError() web.HTTPError {
+	return web.HTTPError{HTTPCode: http.StatusPreconditionFailed, Code: ErrCodeParentProjectIsArchived, Message: "The parent project is archived. Un-archive the parent project first."}
+}
+
 // ==============
 // Task errors
 // ==============

pkg/models/project.go

@@ -1170,6 +1170,13 @@ func RegisterUser(s *xorm.Session, u *user.User) (*user.User, error) {
 	return newUser, nil
 }
 
+func effectiveParentID(project, storedProject *Project) int64 {
+	if project.ParentProjectID != nil {
+		return *project.ParentProjectID
+	}
+	return storedProject.parentID()
+}
+
 // checkProjectParentBeforeUpdate gates reparenting and un-archiving. Both are
 // enforced here and not in CanUpdate: that short-circuits for instance admins
 // and is bypassed entirely by direct UpdateProject callers.
@@ -1180,10 +1187,6 @@ func RegisterUser(s *xorm.Session, u *user.User) (*user.User, error) {
 // grants Admin on the child, and detaching a child to the top level
 // severs an owner's inherited-permission chain. Both are reparent
 // operations that must require Admin on the moved project.
-//
-// ParentProjectID is a *int64 so an omitted parent_project_id (nil) is
-// distinguishable from an explicit 0 (detach-to-root). Only gate when
-// the field was sent and actually changes the parent.
 func checkProjectParentBeforeUpdate(s *xorm.Session, project, storedProject *Project, auth web.Auth) (err error) {
 	isReparent := project.ParentProjectID != nil && *project.ParentProjectID != storedProject.parentID()
 	isUnarchive := storedProject.IsArchived && !project.IsArchived
@@ -1191,10 +1194,7 @@ func checkProjectParentBeforeUpdate(s *xorm.Session, project, storedProject *Pro
 		return nil
 	}
 
-	parentID := storedProject.parentID()
-	if project.ParentProjectID != nil {
-		parentID = *project.ParentProjectID
-	}
+	parentID := effectiveParentID(project, storedProject)
 
 	var parent *Project
 	if parentID > 0 {

pkg/modules/migration/create_from_structure_test.go

@@ -319,6 +319,19 @@ func TestInsertFromStructure(t *testing.T) {
 					ParentProjectID: models.Ptr(int64(2)),
 				},
 			},
+			{
+				Project: models.Project{
+					ID:    4,
+					Title: "Unarchived sibling root",
+				},
+			},
+			{
+				Project: models.Project{
+					ID:              5,
+					Title:           "Unarchived sibling's child",
+					ParentProjectID: models.Ptr(int64(4)),
+				},
+			},
 		}, u))
 
 		s := db.NewSession()
@@ -329,11 +342,15 @@ func TestInsertFromStructure(t *testing.T) {
 			exists, err := s.Where("title = ?", title).Get(project)
 			require.NoError(t, err)
 			require.True(t, exists)
+			assert.True(t, project.IsArchived)
+		}
 
-			db.AssertExists(t, "projects", map[string]interface{}{
-				"id":          project.ID,
-				"is_archived": true,
-			}, false)
+		for _, title := range []string{"Unarchived sibling root", "Unarchived sibling's child"} {
+			project := &models.Project{}
+			exists, err := s.Where("title = ?", title).Get(project)
+			require.NoError(t, err)
+			require.True(t, exists)
+			assert.False(t, project.IsArchived)
 		}
 	})
 	t.Run("keeps positions the export provides", func(t *testing.T) {