| | 1 | [[PageOutline(2-3)]] |
| | 2 | |
| | 3 | = Code Review Wishlist = |
| | 4 | |
| | 5 | This page contains some wishful thinking about a hypothetical ideal code review system built into Trac (or a plugin). |
| | 6 | |
| | 7 | == What to review? == |
| | 8 | |
| | 9 | === Changesets === |
| | 10 | Low overhead. Only for ''post-commit review''. Review experimental repository clones / branches for ''pre-merge review''. |
| | 11 | |
| | 12 | === Files === |
| | 13 | Or parts of files. High overhead. Good for auditing? |
| | 14 | |
| | 15 | === Merge requests === |
| | 16 | Need to add the concept of a merge request to Trac first. But very useful and popular in modern DVCS systems. |
| | 17 | |
| | 18 | === Patches === |
| | 19 | Nice for projects that already rely on submitted patches for their development model. High overhead for projects that are used to direct commit access. |
| | 20 | |
| | 21 | === Screenshots === |
| | 22 | Optional bonus feature? |
| | 23 | |
| | 24 | == Configurable Workflow == |
| | 25 | |
| | 26 | Code reviews should support configurable workflows (like tickets). |
| | 27 | |
| | 28 | === Relevant states === |
| | 29 | * `new` |
| | 30 | * A new code review has been created |
| | 31 | * `closed` |
| | 32 | * The code review has concluded. (Like for tickets, various resolutions may clarify what this means.) |
| | 33 | * `assigned`, `reviewing`, `reviewed`? |
| | 34 | * Shows when a reviewer has been assigned, has started or finished reviewing the code. |
| | 35 | * This may be complicated by the need for multiple reviewers in one review. |
| | 36 | * Avoid over-complicating the process? (It's configurable. Some may need a more advanced workflow, some want to keep a low overhead.) |
| | 37 | |
| | 38 | === Relevant resolutions === |
| | 39 | * `accepted` / `rejected` |
| | 40 | * For ''acceptance code review'' that results in a vote on inclusion. |
| | 41 | * `completed` |
| | 42 | * For ''educational code review'' to increase knowledge transfer. |
| | 43 | * `invalid` (`ignore`, `irrelevant`, `retracted`, `superseded`, `outdated`, `noneed`, `nntr` (''No Need To Review''))) |
| | 44 | * For various cases where a code review is aborted. |
| | 45 | |
| | 46 | === Assigning reviewers === |
| | 47 | A new code review should be assignable to specific reviewers (Trac users) or to a ''group'' of reviewers (permission groups?). Depending on the workflow, one, some or all of them should have to perform some action on the review. |
| | 48 | |
| | 49 | === Voting === |
| | 50 | In some workflows, the individual reviewers are required / allowed to vote on a code review. Voting could be a state transition. A special transition operation could tally and check the votes against a configured threshold. Depending on the outcome it remain in the same state (and wait for further votes) or resolve the review to the `accepted` or `rejected` states. |
| | 51 | |
| | 52 | === Veto === |
| | 53 | Some / all users may have a ''veto'' permission to overrule other votes instantly? |
| | 54 | |
| | 55 | == Commenting == |
| | 56 | Reviewers and submitter need to discuss the reviewed code. Referring to specific section or lines of code should be easy, by attaching and showing comments and replies inline / side-by-side with the code. |
| | 57 | |
| | 58 | == VCS integration == |
| | 59 | * Should a new changeset automatically create a new review? |
| | 60 | * Should a special changeset commit message keyword trigger a new review? (and assign reviewers?) |
| | 61 | * similar to / extending CommitTicketUpdater? |
| | 62 | * Should the existing changeset / diff / repository view pages be extended or modified? |
| | 63 | * If files are reviewable, the browser could show the ''Review status'' of a file. |
| | 64 | * `Unreviewed` link to the form to start a new review. |
| | 65 | * `Reviewed` links to the review. |
| | 66 | * What if only an old revision was reviewed? |
| | 67 | * For directories show the percentage of reviewed files? |
| | 68 | |
| | 69 | == Ticket integration == |
| | 70 | * Should a code review simply be a ticket of ''ticket type'' `codereview`? |
| | 71 | * We get some discussion, notification and workflow support for free. |
| | 72 | * For reviewing patches / merge requests submitted as proposed fixes for the ticket's issue this seems ideal. |
| | 73 | * Should a reviewer open new tickets for each complaint? |
| | 74 | * Should a rejected post-commit code review result in one new ticket? |
| | 75 | |
| | 76 | == New navigation tab == |
| | 77 | If not all code review features are integrated into existing pages, a new navigation tab is needed where code reviews can be created and viewed etc. |
| | 78 | |
| | 79 | One should see (be able to query?) ones submitted and assigned reviews and their most important properties. (Did I or someone else comment or reply?) |
| | 80 | |
| | 81 | Should reviews be prepared and performed in a private ''staging'' area and only published when ready? |
| | 82 | |
| | 83 | == Other Trac integration == |
| | 84 | The code review system should be "natively" integrated in Trac in the sense that it works together with the appropriate core systems: |
| | 85 | * Notification |
| | 86 | * People need to be notified when they are assigned as a reviewer or the status of their code review changes. |
| | 87 | * Timeline |
| | 88 | * The timeline should show the important review events. |
| | 89 | * Search |
| | 90 | * Reviews and review comments should be searchable. |
| | 91 | * Wiki |
| | 92 | * Are there any appropriate special macros, link syntax, etc.? |
| | 93 | * Preferences, Admin, Permissions |
| | 94 | * Configuration of code review permissions, admin settings and user preferences should be provided in the standard Trac way. |
