Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBS-13492: Move beginner status to a privilege flag #3180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented Feb 20, 2024

Implement MBS-13492

Problem

Anything that requires checking whether an editor is a beginner requires us to calculate beginnership every time, which is slow and cumbersome, and implemented in at least three different places in slightly different ways. Specifically, calculating beginnership as part of edit search conditions is awfully, terribly slow.

Solution

Every new editor added via /register will get the BEGINNER_FLAG privilege by default, which will only be removed
when accepting edits once the number of edits + time passed requirements are fulfilled. The flag cannot be set/unset via Edit user, since even admins should have no reason to override the default mechanism for this.

We calculate this in _do_accept, rather than the edit queue itself, because approvals skip the edit queue but are still relevant for this check.

We run a one-off script to set the flag on any existing beginners.

Testing

Registered a new user and saw it does get the Beginner flag. Added a test to make sure the EditQueue processing removes the beginner flag when appropriate.

Draft progress

  • Add a cron job so that the beginner flag is eventually removed (@mwiencek suggested running this as part of CheckVotes on a per-edit basis, since we have the editor loaded and we can check their privs and member_since without having to query them, so we would only need to check if they have the required amount of edits and then unset their beginner flag).
  • Add a one-off script to set the beginner flag on every beginner account that already exists, after which they'll just work with the cron job like others.
  • Remove extra cruft from tests (such as SQL inserting 10 nonsense edits so that the editor is not a beginner anymore)

@reosarevok reosarevok added the Refactoring Refactoring-only PRs (eslint fixes etc) label Feb 20, 2024
@reosarevok reosarevok force-pushed the MBS-13492 branch 4 times, most recently from 8f86426 to 4784bf7 Compare February 21, 2024 11:46
@reosarevok reosarevok marked this pull request as ready for review February 21, 2024 11:47
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag cannot be set/unset via Edit user, since even admins should have no reason to override the default mechanism for this.

Would that affect bot users in any way?

lib/MusicBrainz/Server/EditQueue.pm Outdated Show resolved Hide resolved
@reosarevok
Copy link
Member Author

reosarevok commented Mar 1, 2024

Well, we do not actually have any special mechanism for beginner bots at the moment :) I expect even just the test edits would be enough for a bot to stop being a beginner, in most cases. But in any case, since bots shouldn't vote, it shouldn't matter?

This stops us having to calculate beginnership every time, which
is slow and cumbersome, and implemented in at least three
different places in slightly different ways.

Every new editor added via /register will get the BEGINNER_FLAG
privilege by default, which will only be removed
when accepting edits once the number of edits + time passed
requirements are fulfilled.

We will need to run a one-off script to set the flag
on any existing beginners, added as 20240221-mbs-13492.sql.

The flag cannot be set/unset via Edit user, since even admins
should have no reason to override the default mechanism for this.

One small benefit of this is we can better control who is
a beginner for testing purposes, so we no longer need to add
dummy edits just to allow a test editor to vote.

We calculate this in _do_accept, rather than the edit queue itself,
because approvals skip the edit queue but are still relevant
for this check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring Refactoring-only PRs (eslint fixes etc)
Projects
None yet
2 participants