Ignore archived repos in one-article-per-subject rule - #354
Conversation
* CheckForkOnEditPermissions and ForkRepository only treat an active (non-archived) repo for the subject as blocking * GetRepositoryByOwnerIDAndSubjectID now prefers active over archived rows deterministically, since a user can hold both * add ArchivedOwnArticleDoesNotBlock unit test
|
/review AI review started. |
Advanced AI Review
Click to expand reviewReview: Ignore archived repos in one-article-per-subject ruleThe core fix is sound end-to-end (the editor fork path picks a collision-free name via 1. 🟡 DRY: the "archived doesn't count" policy is duplicated at two call sites instead of living in the model
The same exemption logic now exists in two places, and its correctness silently depends on a third place (the // services/repository/fork.go:105-110
// An archived repository is not an active article, so it does not consume the
// user's "one article per subject" slot: treat it as if the user had no repo
// for the subject.
if ownRepo != nil && ownRepo.IsArchived {
ownRepo = nil
}// services/repository/fork.go:307
if ownRepo != nil && !ownRepo.IsArchived && ownRepo.ID != opts.BaseRepo.ID {...combined with the ordering contract at OrderBy("is_archived ASC, id ASC").Both fork.go call sites need "the owner's active article for this subject", but each must remember to check // models/repo/repo.go
// GetActiveRepositoryByOwnerIDAndSubjectID returns the owner's active (non-archived)
// repository for the given subject, or nil if the owner has none (or only archived ones).
func GetActiveRepositoryByOwnerIDAndSubjectID(ctx context.Context, ownerID, subjectID int64) (*Repository, error) {
var repo Repository
has, err := db.GetEngine(ctx).
Where("owner_id = ?", ownerID).
And("subject_id = ?", subjectID).
And("is_archived = ?", false).
OrderBy("id ASC").
Get(&repo)
if err != nil || !has {
return nil, err
}
return &repo, nil
}Then in // services/repository/fork.go:88
ownRepo, err = repo_model.GetActiveRepositoryByOwnerIDAndSubjectID(gCtx, doer.ID, repo.SubjectID)and in // services/repository/fork.go:307
if ownRepo != nil && ownRepo.ID != opts.BaseRepo.ID {The transfer pre-check at 2. 🟡 The actual
|
* add ForkSucceedsAfterArchivingOwnArticle integration subtest exercising the guard in ForkRepository * add model tests pinning the is_archived ASC, id ASC preference in GetRepositoryByOwnerIDAndSubjectID
* handleForkAndEdit returns repo.editor.existing_fork_archived instead of committing into an archived fork * exempt the fork-and-edit workflow from the current-repo protected-branch and maintainer-write checks (_edit/_new only), which previously masked the archived case
* add GetActiveRepositoryByOwnerIDAndSubjectID and use it for the transfer pre-check * restrict ExcludeOwnersOfSubjectID to active articles so the candidate search matches the pre-check
* assert fixture ownership in ArchivedOwnArticleDoesNotBlock, matching BlockedBySubjectOwnership * pin HasExistingFork == false so the Case-1 branch is the one exercised
* order GetRepositoryByOwnerAndSubject by is_archived ASC, updated_unix DESC, id DESC so the URL follows the newest active article and falls back to archived only when none is active. * add coverage for active-wins, archived-fallback and not-found cases. * trade-off: GetRepositoryByOwnerIDAndSubjectID keeps its id ASC tiebreak, since fork-permission logic depends on it.
* Home renders the Article View for subject-bound repositories, resolving to that exact repository * article tabs and JS navigation follow ArticleLink/data-article-canonical instead of the hardcoded vanity path * new integration test covers both routes, sub-paths, and archived articles
* Repository.Link() returns the permanent URL for archived subject repositories * table rows, bubble nodes and stored selections carry the archived flag; buildArticleUrl emits /owner/repo?view=article for them * trade-off: no query string on Link(), since it is concatenated as a path prefix across the codebase
* Repository.Link() returns /article/{owner}/{repo} for archived subject repositories
* new GetArticleRepositoryByOwnerAndRef resolves the article path element as subject name first, repository name second
* buildArticleUrl emits the prefixed permanent URL for archived selections
* trade-off: no ?view=article on Link(), which is used as a path prefix across the codebase
* Link() short-circuits to OperationsLink() for archived subject repos, dropping the ambiguous /article/{owner}/{name} fallback and its DB query
* frontend and ArticleView follow the repo link, canonical path preserved across read/history navigation
* integration tests restore archive state and cover the name-vs-slug collision
* CanWriteToBranch reads fork_and_edit and submit_change_request with ctx.FormBool, matching the editor handler * action allowlist unchanged, so the _delete/_upload/_diffpatch bypass boundary still holds
* ExistingForkArchived added to ForkOnEditPermissions and used by both the fork-and-edit POST and the article page * editor GET returns the archived-specific message so the edit is not written first
Advanced AI Review
Click to expand review1. 🔴 Permanent article URL can resolve to a different repository
repo, err := GetRepositoryByOwnerAndSubject(ctx, ownerName, ref)
if err == nil {
return repo, nil
}Article repository names are generated through For example:
The archived article therefore becomes unreachable through its own permanent link, which silently serves a different article. The The implemented resolution makes archived articles use their operations URL: func (repo *Repository) Link() string {
if repo.IsArchived && repo.SubjectID > 0 {
// The subject vanity URL resolves to the active repository of the subject.
return repo.OperationsLink()
}
return setting.AppSubURL +
"/article/" +
url.PathEscape(repo.OwnerName) +
"/" +
url.PathEscape(repo.GetSubject(context.Background()))
}
2. 🟡 Subject-bound repositories no longer use the repository code view
Consequently, This appears intentional because, in Forkana, a subject-bound repository is an article. The remaining concern is primarily explanatory: the function comment attributes the behavior to archived-article reachability without describing its effect on every subject-bound repository. A related inconsistency exists in 3. 🟡
|
There was a problem hiding this comment.
🟡 Changes recommended
The repo-history localStorage overwrite check can leave a stale archived flag, which can route an archived article through the vanity subject URL and open the wrong repository.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
|
/review AI review started. |
Advanced AI Review
Click to expand reviewThe full PR is commits 1. 🟡
|
* CanWriteToBranch and prepareEditorCommitSubmittedForm no longer re-parse fork_and_edit/submit_change_request with ctx.FormBool, so the gates and the handler always agree.
* Flags exposed via forms.EditorWorkflowForm; services/context mirrors it as an unexported interface to avoid an import cycle.
* Regression tests for binder-rejected values ("On"), which previously bypassed both gates and reached the direct-commit path.
* CheckForkOnEditPermissions and ForkRepository now call GetActiveRepositoryByOwnerIDAndSubjectID, dropping the manual archived nil-out and the inline !ownRepo.IsArchived check, so all three subject-slot call sites share the same database-level filter. * Removed GetRepositoryByOwnerIDAndSubjectID, which had no callers left, and its ordering test; the active getter's test absorbed the "archived repo has the lower ID" coverage. * Retargeted the v327 index comments to the surviving getter and its is_archived predicate; the (owner_id, subject_id) index is unchanged.
* RepoAssignmentByOwnerAndSubject no longer claims archived articles are unaddressable; it documents the active-preferred lookup with the archived fallback. * narrowed the matching claim in TestSubjectLookupPrefersActiveRepository to what the assertion covers.
* removed ExistingForkArchived writes from both ownRepo != nil branches, where the value is always false; the live Case-1 write stays.
* extracted the four localStorage keys and the read/write/clear logic into web_src/js/modules/repo-selection.ts; repo-history.ts and FishboneGraph.vue import it. * merged the two divergent copies: the graph's missing-window guard and the history view's subject-only read fallback are both retained.
* $articleLink now reads .ArticleLink only * the printf fallback hard-coded the vanity URL, wrong for an archived article, and escaped the subject with PathEscapeSegments against the Go side's PathEscape.
* ArticleView now builds ArticleLink straight from the subjectname path param; the subject == branch could not be reached from its only route, and was a no-op even in principle since the repository is resolved by that same segment. * added TestArticlePermanentRoute/ArticleURLWithoutSubjectDoesNotReachTheArticleView to pin that premise.
* GetActiveRepositoryByOwnerIDAndSubjectID now tie-breaks on updated_unix DESC, id DESC, matching GetRepositoryByOwnerAndSubject, so the subject-slot check and the vanity URL pick the same repository if an owner ends up with several active ones. * Link()'s comment now states its two real branches; the "falls back to repository name" claim was moved onto the branch where GetSubject makes it true. * Deliberately omitted the suggested is_archived ASC prefix: the query already filters on that column, so it would sort a constant.
|
@taoeffect ready! ✅ |
Fixes #338
AI Disclosure
Co-authored with: Opus 5