Make a distinction between moderators and admins #619

Merged
elegaanz merged 10 commits from moderator-role into master 5 years ago
elegaanz commented 5 years ago (Migrated from github.com)

And rework the user list in the moderation interface, to be able to run the same action on many users, and to have a huge list of actions without loosing space.

Peek 18-06-2019 18-36

Fixes #582, fixes #359

And rework the user list in the moderation interface, to be able to run the same action on many users, and to have a huge list of actions without loosing space. ![Peek 18-06-2019 18-36](https://user-images.githubusercontent.com/16254623/59708839-7d82fa80-91fd-11e9-8041-07b39d39ae2c.gif) Fixes #582, fixes #359
trinity-1686a reviewed 5 years ago
trinity-1686a left a comment
Owner

I think we can assume an administrator is always a moderator (anyway if they are not, they can just grant themselves), so it might be more logical to have an enum user/mod/admin

(also I think UI for user edition should be a drop-down per user, with their current role as default value, and possibility to change their actual role, but I'm far from good at the whole UI thing)

(I request change for the incorrect route, the rest is more of a discussion with no strong positions)

I think we can assume an administrator is always a moderator (anyway if they are not, they can just grant themselves), so it might be more logical to have an enum user/mod/admin (also I think UI for user edition should be a drop-down per user, with their current role as default value, and possibility to change their actual role, but I'm far from good at the whole UI thing) (I request change for the incorrect route, the rest is more of a discussion with no strong positions)
@ -190,3 +205,3 @@
_admin: Admin,
_mod: Moderator,
page: Option<Page>,
rockets: PlumeRocket,
Owner

if I'm reading this right, and admin can't actually edit users, and a moderator can grant themselves admin rights

if I'm reading this right, and admin can't actually edit users, and a moderator can grant themselves admin rights
elegaanz (Migrated from github.com) reviewed 5 years ago
@ -190,3 +205,3 @@
_admin: Admin,
_mod: Moderator,
page: Option<Page>,
rockets: PlumeRocket,
elegaanz (Migrated from github.com) commented 5 years ago

Indeed.

I will try to make permissions an enum as you suggested.

For the UI part, the disadvantage of your proposal is that you can't apply the same action to a lot of users at the same time (but I don't know if it is really needed?).

Indeed. I will try to make permissions an enum as you suggested. For the UI part, the disadvantage of your proposal is that you can't apply the same action to a lot of users at the same time (but I don't know if it is really needed?).
trinity-1686a reviewed 5 years ago
@ -190,3 +205,3 @@
_admin: Admin,
_mod: Moderator,
page: Option<Page>,
rockets: PlumeRocket,
Owner

maybe for mass-ban, but probably not for mass-granting-privileges

maybe for mass-ban, but probably not for mass-granting-privileges
codecov[bot] commented 5 years ago (Migrated from github.com)

Codecov Report

Merging #619 into master will decrease coverage by 0.27%.
The diff coverage is 48.57%.

@@            Coverage Diff            @@
##           master    #619      +/-   ##
=========================================
- Coverage   34.58%   34.3%   -0.28%     
=========================================
  Files          68      68              
  Lines        8020    8094      +74     
  Branches     1890    1920      +30     
=========================================
+ Hits         2774    2777       +3     
- Misses       4468    4537      +69     
- Partials      778     780       +2
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/619?src=pr&el=h1) Report > Merging [#619](https://codecov.io/gh/Plume-org/Plume/pull/619?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/d46af6fe5bb472d0aa9435520690c3b94a3ec577?src=pr&el=desc) will **decrease** coverage by `0.27%`. > The diff coverage is `48.57%`. ```diff @@ Coverage Diff @@ ## master #619 +/- ## ========================================= - Coverage 34.58% 34.3% -0.28% ========================================= Files 68 68 Lines 8020 8094 +74 Branches 1890 1920 +30 ========================================= + Hits 2774 2777 +3 - Misses 4468 4537 +69 - Partials 778 780 +2 ```
igalic (Migrated from github.com) reviewed 5 years ago
igalic (Migrated from github.com) left a comment

not quite a 👀

the did diff is impossible to review on mobile thanks to translations, which, too, are almost impossible to review, so i'm not sure it makes sense that we commit them as part of regular PRs

not quite a 👀 the did diff is impossible to review on mobile thanks to translations, which, too, are almost impossible to review, so i'm not sure it makes sense that we commit them as part of regular PRs
igalic (Migrated from github.com) commented 5 years ago

perhaps, "Author"?

perhaps, "Author"?
trinity-1686a reviewed 5 years ago
Owner

I don't know, this also includes someone who made an account just to comment

I don't know, this also includes someone who made an account just to comment
elegaanz commented 5 years ago (Migrated from github.com)

For translations, I think I will remove them for the moment. Once we will switch to the 2018 edition, I think we will be able to use the latest version of gettext-macro which has options to make smaller diffs.

For translations, I think I will remove them for the moment. Once we will switch to the 2018 edition, I think we will be able to use the latest version of `gettext-macro` which has options to make smaller diffs.
igalic (Migrated from github.com) reviewed 5 years ago
igalic (Migrated from github.com) left a comment

sleepy 👀

sleepy 👀
igalic (Migrated from github.com) commented 5 years ago

that means, on this end, we'd just have a (one) from_str()

that means, on this end, we'd just have a (one) `from_str()`
igalic (Migrated from github.com) commented 5 years ago

can't we use an enum here instead of these numbers? (or in SQLite, a check constraint https://stackoverflow.com/questions/5299267/how-to-create-enum-type-in-sqlite)

can't we use an enum here instead of these numbers? (or in SQLite, a check constraint https://stackoverflow.com/questions/5299267/how-to-create-enum-type-in-sqlite)
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

See this commit message e3ecbdb2f4

I tried to use a crate to do that with Diesel, but it generates a schema with the enum type for Postgres and with Text for SQlite, which we would have to maintain two different User struct depending on the database, one with role: UserRole, and the other with role: String,, and I don't want to do that.

See this commit message https://github.com/Plume-org/Plume/pull/619/commits/e3ecbdb2f4a8aa2b857057b27cd75b44c379767b I tried to use a crate to do that with Diesel, but it generates a schema with the enum type for Postgres and with `Text` for SQlite, which we would have to maintain two different `User` struct depending on the database, one with `role: UserRole,` and the other with `role: String,`, and I don't want to do that.
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

Sorry, but… I don't understand. 😅 Could you try to rephrase, please?

Sorry, but… I don't understand. :sweat_smile: Could you try to rephrase, please?
trinity-1686a reviewed 5 years ago
Owner

Having an enum to make the comparison and not having magic numbers everywhere would still be cool

enum Role {
    Admin = 0,
    Mod = 1,
    User = 2,
}
fn main() {
    println!("{}", 0 == Role::Admin as u32)
}
Having an enum to make the comparison and not having magic numbers everywhere would still be cool ```rust enum Role { Admin = 0, Mod = 1, User = 2, } fn main() { println!("{}", 0 == Role::Admin as u32) } ```
elegaanz (Migrated from github.com) reviewed 5 years ago
elegaanz (Migrated from github.com) commented 5 years ago

I didn't know it was possible to associate numbers to enum variants like that.

I didn't know it was possible to associate numbers to enum variants like that.
igalic (Migrated from github.com) reviewed 5 years ago
igalic (Migrated from github.com) left a comment

@ -51,6 +51,12 @@ use {ap_url, Connection, Error, PlumeRocket, Result};
pub type CustomPerson = CustomObject<ApSignature, Person>;
pub enum Role {
igalic (Migrated from github.com) commented 5 years ago

if we #[derive(Shrinkwrap)] would it auto-convert as i32?

if we `#[derive(Shrinkwrap)]` would it auto-convert `as i32`?
trinity-1686a reviewed 5 years ago
@ -51,6 +51,12 @@ use {ap_url, Connection, Error, PlumeRocket, Result};
pub type CustomPerson = CustomObject<ApSignature, Person>;
pub enum Role {
Owner

It's not a struct Role(i32), so I don't think so. Maybe tinkering with one of Deref, Borrow or AsRef can make it less verbose (like requiring only a & maybe)
An ugly but functional way of doing so would be to that:

pub mod Role {
    pub const Admin: i32 = 0;
    pub const Mod: i32 = 1;
    pub const User: i32 = 2;
}

It won't work for matching and won't protect from invalid value as effectively as an enum, but it does not require as i32

It's not a struct Role(i32), so I don't think so. Maybe tinkering with one of Deref, Borrow or AsRef can make it less verbose (like requiring only a & maybe) An ugly but functional way of doing so would be to that: ```rust pub mod Role { pub const Admin: i32 = 0; pub const Mod: i32 = 1; pub const User: i32 = 2; } ``` It won't work for matching and won't protect from invalid value as effectively as an enum, but it does not require as i32
trinity-1686a reviewed 5 years ago
@ -51,6 +51,12 @@ use {ap_url, Connection, Error, PlumeRocket, Result};
pub type CustomPerson = CustomObject<ApSignature, Person>;
pub enum Role {
Owner

thinking about it, this is a thing, and is exactly what you hopped to do with Shinkwrap

thinking about it, [this](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4b2a159c18dbea965ddc2dc504571b93) is a thing, and is exactly what you hopped to do with Shinkwrap
trinity-1686a approved these changes 5 years ago

Reviewers

trinity-1686a approved these changes 5 years ago
The pull request has been merged as 309e1200d0.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b moderator-role master
git pull origin moderator-role

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff moderator-role
git push origin master
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#619
Loading…
There is no content yet.