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

SQL: Schema Changes for Administration System #993

Merged

Conversation

the-good-boy
Copy link
Contributor

This PR deals with the schema changes for the Administration System.

Changes:

  • A new column called privs added to bookbrainz.editor.
  • A new type called admin_action_type has been added to support potential action types which can be added in the future
  • A new table called bookbrainz.admin_log has been created to track admin logs

Comment on lines 13 to 14
user1_id INT NOT NULL,
user2_id INT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
user1_id INT NOT NULL,
user2_id INT NOT NULL,
admin_user_id INT NOT NULL,
affected_user_id INT NOT NULL,

Numbering the involved users makes these tables harder to understand, I would suggest naming the columns after the users' roles. Since this has also to be changed in other places, it would probably make sense to await all reviews before doing the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I am hesitating to use such naming convention is because admin_user sounds like a higher-ranked user in hierarchy compared to the second user. This will be true in the current use-case. But in the future, when more actions such as Reporting a user are added, the first user might not necessarily be higher ranked than the second one.
It is possible that I might be reading too much into this.
For what its worth, my initial proposed schema contained similar naming. I am fine with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, the affected user could be an "admin" too (whatever that means in the future). Maybe acting_user would be a better name?

Copy link
Contributor Author

@the-good-boy the-good-boy Jun 5, 2023

Choose a reason for hiding this comment

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

I think we can go with a simple admin_id and user_id columns. Since an 'Admin' is not an explicit user type in bookbrainz, we can say that an admin refers to anyone who is performing the administrative action, and the user is the user to which the action is directed at.
The moderation log schema of CritiqueBrainz is also somewhat in the same vein, so I think this will be good enough. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think admin_id and target_user_id would remove any doubt as to who is doing what, and might be a bit clearer than "actor"/"acting" or other alternatives I could think of (executor, initiator).

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using user_id for the user initiating the action and target_id for the user getting affected?

Copy link
Member

Choose a reason for hiding this comment

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

I think that is more confusing at a quick glance, user_id could be either of the two.
Obviously target_id or target_user_id is quite clear, so perhaps source_user_id could also work in that case (but I still prefer admin_id myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also inclined towards using admin_id and target_user_id. I think they are succinct and serve our purpose well.

Copy link
Member

Choose a reason for hiding this comment

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

Also relevant, for what it's worth: I don't think the admin logs will show any action that hasn't been taken by an "admin".
For example, the use-case cited above (user1 reports user2) likely won't be shown in the admin logs, but the resulting action (admin blocked user2) would be.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Apart from the comments below, looks all good !

Excited to see the start of the project ! 🚀

Comment on lines 13 to 14
user1_id INT NOT NULL,
user2_id INT NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

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

I think admin_id and target_user_id would remove any doubt as to who is doing what, and might be a bit clearer than "actor"/"acting" or other alternatives I could think of (executor, initiator).

sql/schemas/bookbrainz.sql Outdated Show resolved Hide resolved
@MonkeyDo MonkeyDo changed the base branch from master to administration-system June 5, 2023 16:09
@MonkeyDo
Copy link
Member

MonkeyDo commented Jun 5, 2023

I changed the merge target to the newly created administration-system feature branch.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Good to go! 🚀

And so it begins !

I ran both migration files against the test.bb.org database, for reference, so that it is ready for the next steps.

@MonkeyDo MonkeyDo merged commit 119d9dc into metabrainz:administration-system Jun 6, 2023
MonkeyDo added a commit to metabrainz/bookbrainz-data-js that referenced this pull request Sep 22, 2023
the 'privs' column was added in metabrainz/bookbrainz-site#993 and merged into bookbrainz-site master branch in metabrainz/bookbrainz-site#1020
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.

5 participants