Disregard contributor count prior to article fork #100
Conversation
* set fork-related context data * show submit button based on permissions * add ForkAndEdit field to EditRepoFileForm * modify EditFilePost to handle fork creation and commit to fork * reuse existing fork if user already has one
* query repos by owner and subject * check for existing user repo for subject * disable edit/fork buttons when user already has a repo for the subject * show message in edit view explaining why editing is disabled
* fix wrong error condition (returned on any error, not just 'not found') * fix double increment bug (i++ in loop body + loop increment) * optimise from O(n) queries to single query with hash set lookup O(1) * add proper error logging
* run subject ownership check and fork detection concurrently
* add error for subject ownership violations * validate subject ownership before forking
* fix POST handler to validate subject ownership
* create IDX_repository_owner_subject and IDX_repository_owner_fork * optimizes GetRepositoryByOwnerIDAndSubjectID() and GetForkedRepo() queries
* replace 'Cannot Edit' button with 'Edit Your Article' action link * add translation keys for fork-on-edit error messages * link users to their existing article when subject ownership blocks editing
* simplify fork_and_edit hidden field to only use .NeedsFork * extract repeated nil-guard pattern into $subjectName template variable
* add Playwright tests for modal appearance, cancel actions, and tooltip * test modal shows correct header, content, and button text * verify cancel button and Escape key close modal without action * verify fork button has tooltip with confirmation message
* add repository owner tests for Submit Changes button behavior * add non-owner tests for Fork button and disabled submit button * add unauthenticated user test for sign-in button
* accessibility tests verify Enter opens modal, Tab navigates buttons, and buttons have proper accessible text content * unique name tests verify suffix generation when base name is taken
* use RepoOperationsLink instead of RepoLink in form action url * bypass fork_and_edit in CanWriteToBranch middleware to allow non-owners to reach the EditFilePost handler * skip NeedFork workflow when ForkAndEdit is true to prevent conflicting fork creation attempts
* CheckForkOnEditPermissions for owner/non-owner/unauthenticated * CanWriteToBranch middleware bypass with fork_and_edit=true * existing fork detection using fixture data (repo11/repo10) * subject ownership blocking prevents duplicate forks * form action URL uses RepoOperationsLink not RepoLink
* slug generation migration covering basic slugs, mixed-case deduplication, special characters, unique constraint verification, and empty table handling * index creation, idempotency, and index type verification
…order * fork API behavior when user already owns a repository for the same subject * fork blocked by subject ownership (403), fork succeeds for user without subject repo (202), fork blocked even with custom name (403) * fix error handling order in CreateFork
* update GetForkedRepo signature to return (*Repository, error) * update all callers to handle the new error return value * fixes silent error swallowing
* fix modal promise resolution pattern * add PathEscapeSegments to article template URL * update test comment to list all removed special characters
* replace page.waitForSelector() with expect(locator).toBeAttached() to satisfy playwright/no-wait-for-selector rule * fix accessibility test to use proper Playwright assertions instead of treating Locators as strings
* toast UI Editor creates two .toastui-editor elements (md-mode and ww-mode) * use .first() to satisfy Playwright's strict mode requirement
* add maxUniqueNameAttempts constant to replace magic number 1000 * use t.Cleanup with nil check instead of defer for safer test cleanup * remove unused data-existing-fork attribute from article template * add comment explaining that subject ownership check takes precedence
* add since parameter to GetContributorCount, getFileContributorCount and getContributorStats * filter both TotalCount and RecentCount by fork creation time * for forks, only count contributors who made commits after fork creation * prevents inherited parent history from inflating fork contributor counts * add tests for GetContributorCount with since filtering
* add hasCommitsAfter function to check if contributor has commits after a given time
* check if week *ends* after since time instead of week *starts* after * ensure mid-week fork creation includes commits from later in that week * fix edge case where post-fork commits were not counted
* add comprehensive test for hasCommitsAfter helper function * cover mid-week fork creation edge case * test boundary conditions at week start/end * test zero since time and empty contributor cases
There was a problem hiding this comment.
Pull request overview
This PR modifies contributor counting for forked repositories to exclude contributors from the inherited history before the fork was created. The changes apply time-based filtering using the fork creation timestamp as the cutoff point.
- Adds a
sinceparameter to contributor counting functions to filter by time - Updates time format to RFC3339 for git command compatibility
- Implements week-based filtering logic for cached contributor statistics
- Adds comprehensive tests for edge cases including mid-week fork creation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
services/repository/fork_graph.go |
Adds hasCommitsAfter helper and modifies getContributorStats to filter contributors by fork creation time using week-based granularity |
services/repository/fork_graph_test.go |
Adds tests for getContributorStats with time filtering and hasCommitsAfter function including mid-week edge cases |
services/context/repo.go |
Updates RepoAssignmentBySubject to pass fork creation time when counting contributors for forked repositories |
routers/web/explore/repo.go |
Modifies getFileContributorCount and RenderRepositoryHistory to apply fork time filtering for contributor counts |
modules/git/repo_stats.go |
Updates GetContributorCount signature to accept a since parameter and adds --since flag with RFC3339 formatting to git commands |
modules/git/repo_stats_test.go |
Adds tests for GetContributorCount with various time filters including future and past dates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // hasCommitsAfter checks if a contributor has any commits after the given time. | ||
| // Returns true if since is zero (no filtering) or if the contributor has at least one commit after since. | ||
| // Note: Since week.Week is the Unix timestamp of the start of the week (Sunday), we check if the | ||
| // week *ends* after since to include commits made mid-week after a fork created earlier that week. |
There was a problem hiding this comment.
The documentation states this function checks if a contributor has commits "after" the given time, but due to week-based granularity, this is not entirely accurate. The function will include contributors who have commits in any week that overlaps with the post-fork period, even if all their commits in that week were made before the fork creation time.
Consider updating the documentation to clarify this limitation: "Returns true if the contributor has any week with commits that overlaps with or occurs after the since time. Note: Due to week-based granularity, this may include commits made before the since time if they fall within a week that extends past since."
| // hasCommitsAfter checks if a contributor has any commits after the given time. | |
| // Returns true if since is zero (no filtering) or if the contributor has at least one commit after since. | |
| // Note: Since week.Week is the Unix timestamp of the start of the week (Sunday), we check if the | |
| // week *ends* after since to include commits made mid-week after a fork created earlier that week. | |
| // hasCommitsAfter checks if a contributor has any week of commits that overlaps with or occurs | |
| // after the given time. | |
| // Returns true if since is zero (no filtering) or if the contributor has at least one week with | |
| // commits whose week-end is after since. Note: Since week.Week is the Unix timestamp of the start | |
| // of the week (Sunday), this week-based granularity may include commits made before since if they | |
| // fall within a week that extends past since. |
| // This is the key edge case: fork created on Wednesday, commits exist in that week | ||
| // The week started Sunday (before fork) but ends Sunday (after fork) | ||
| // So commits made Thursday-Saturday should be counted |
There was a problem hiding this comment.
The comment "So commits made Thursday-Saturday should be counted" is misleading. Due to the week-based granularity of the contributor data, the function cannot distinguish between commits made on different days within the same week. It will count ALL commits in the week (Sunday-Saturday, including those before the Wednesday fork) if the week overlaps with the post-fork period, not just Thursday-Saturday commits.
| // This is the key edge case: fork created on Wednesday, commits exist in that week | |
| // The week started Sunday (before fork) but ends Sunday (after fork) | |
| // So commits made Thursday-Saturday should be counted | |
| // This is the key edge case: fork created on Wednesday, commits exist in that week. | |
| // Contributor data is week-based (Sunday-Saturday). If the week overlaps the | |
| // post-fork period at all, all commits in that week are treated as post-fork | |
| // for hasCommitsAfter, even though we cannot distinguish which specific days | |
| // within the week the commits occurred. |
| // If since is non-zero, only counts contributors who made commits after that time. | ||
| // This is useful for forks where we only want to count post-fork contributions. |
There was a problem hiding this comment.
The documentation states this function "only counts contributors who made commits after that time", but due to week-based granularity of the contributor data, this is not entirely accurate. It will count contributors who have commits in any week that overlaps with the post-fork period, even if some or all commits were made before the fork.
Consider clarifying: "If since is non-zero, only counts contributors who have at least one week with commits that overlaps with or occurs after that time. Note: Due to week-based granularity, this may include some commits made before the since time."
| // If since is non-zero, only counts contributors who made commits after that time. | |
| // This is useful for forks where we only want to count post-fork contributions. | |
| // If since is non-zero, only counts contributors who have at least one week with commits | |
| // that overlaps with or occurs after that time. This is useful for forks where we only | |
| // want to count post-fork contributions. Note: Due to week-based granularity, this may | |
| // include some commits made before the since time. |
| // For forks, only count contributors who made commits after the fork was created | ||
| // to exclude inherited history from the parent repository |
There was a problem hiding this comment.
The comment states "only count contributors who made commits after the fork was created", but due to week-based granularity in the contributor data, this may include contributors who have commits in weeks that overlap with the fork creation time, even if some commits were before the fork. Consider clarifying: "only count contributors who have commits in weeks that overlap with or occur after the fork was created".
| // For forks, only count contributors who made commits after the fork was created | |
| // to exclude inherited history from the parent repository | |
| // For forks, only count contributors who have commits in weeks that overlap with | |
| // or occur after the fork's creation time, to minimize inherited history from the parent |
| // For forks, only count contributors who made commits after the fork was created | ||
| // to exclude inherited history from the parent repository |
There was a problem hiding this comment.
The comment states "only count contributors who made commits after the fork was created", but due to week-based granularity in the contributor data, this may include contributors who have commits in weeks that overlap with the fork creation time, even if some commits were before the fork. Consider clarifying: "only count contributors who have commits in weeks that overlap with or occur after the fork was created".
| // For forks, only count contributors who made commits after the fork was created | |
| // to exclude inherited history from the parent repository | |
| // For forks, only count contributors who have commits in weeks that overlap with | |
| // or occur after the fork's creation time, to approximate excluding inherited | |
| // history from the parent repository |
pedrogaudencio
changed the title
--sinceflagCloses /issues/87