Skip to content

Conversation

@yannham
Copy link
Contributor

@yannham yannham commented May 20, 2025

Closes #417. Supersedes #554.

This PR implements the "remove maintainer" button for draft issues. In order to make that easier, we introduced a proper maintainer component (everything was handled directly, inlined, in maintainers_list before) and a notion of selectable maintainer, as it exists already for packages. In order to have things consistent between packages and maintainers, we refactored a bit the former: instead of duplicating the code into a normal component and a selectable_xxx one, the same component handles both with a boolean parameter. Inclusion tags for the selectable version are still provided for convenience.

As discussed in the matrix channel, the edits are implemented as a diff action: removing a maintainer stores somewhere a "remove" edit in the base, and that is taken into account to regenerate the list of maintainers of the cached suggestion. The main motivation is that the base set of maintainers might evolve with time (that is, with updates to Nixpkgs), so we want to be able to re-apply the edits on a new maintainer basis.

Because of the diff nature of editing maintainers, we didn't go for the same logic used for packages, where "removing" a package corresponds to unticking a checkbox. It's adapted to packages because we just store the data as "a set of packages"; ticking or unticking boxes include or uninclude stuff. But since we want to store diffs, we would have to recompute said diff on the backend from two sets of maintainers (and it's not trivial to handle, because "removing" a maintainer can also mean "unadding" a maintainer).

Instead, the "trash" icon is an actual button that is handled by a separate POST endpoint whose job is to toggle the presence of the maintainer in the list. It then answers an updated selectable_maintainer component which is the same as before but with some boolean flag set, which shows an undo button and the strike-through style. Each time a maintainer is edited, the list of maintainers of the cached suggestion is regenerated. Since create_gh_issue takes this list as the source of truth, no further modifications are required: publishing an issue takes the edits into account by definition.

TODO

Left for future work (as this PR is already growing big)

  • Populate the activity log when editing maintainers

@fricklerhandwerk
Copy link
Collaborator

Would be good if we can grey out the whole text, it currently looks weird on Firefox

image

@yannham
Copy link
Contributor Author

yannham commented May 26, 2025

@fricklerhandwerk fixed in 1bb618f.

@fricklerhandwerk
Copy link
Collaborator

fricklerhandwerk commented May 26, 2025

Nice! The issue doesn't say it precisely (that was an oversight on my end), but let's make maintainers removable in the untriaged state as well. Petty good other than that.

@yannham
Copy link
Contributor Author

yannham commented May 26, 2025

Done in 7ff1b12

@yannham
Copy link
Contributor Author

yannham commented May 26, 2025

@fricklerhandwerk I'll let you click accept before merging this one.

@yannham yannham merged commit 316a7e3 into NixOS:main May 26, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove package maintainers from issue draft

2 participants