-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
5324 create new role moderator #6351
5324 create new role moderator #6351
Conversation
@@ -150,7 +157,7 @@ def gon_set_current_user | |||
return unless user_signed_in? | |||
a_ids = session[:a_ids] || [] | |||
user = UserPresenter.new(current_user, a_ids) | |||
gon.push({:user => user}) | |||
gon.push({user: user}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant curly braces around a hash parameter.
Awesome <3 |
@@ -94,6 +94,9 @@ | |||
{{#if current_user.admin}} | |||
<li><a href="/admins/dashboard">{{t "header.admin"}}</a></li> | |||
{{/if}} | |||
{{#if current_user.moderator}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rather use obvious predicate methods here:
current_user.moderator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a Handlebars template rendered on the client side, not Ruby ;)
a84c411
to
76e533e
Compare
Ok this should be ready to merge. |
@@ -17,6 +18,14 @@ def self.add_admin(person) | |||
find_or_create_by(person_id: person.id, name: "admin") | |||
end | |||
|
|||
def self.moderator?(person) | |||
exists?(person_id: person.id, name: "moderator") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moderators.exists?(person_id: person.id)
and then the || current_user.admin?
above shouldn't be needed anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then three specs would be awesome, expect(Role.moderator?(admin_person)).to be_true
, expect(Role.moderator?(moderator_person)).to be_true
, expect(Role.moderator?(normal_person)).to be_false
We added the specs and refactored the method. what do you think? |
I think we're almost done here. I'd like the three specs for |
@jhass do you mean in the same document but outside the scope? |
it "should not include normal users" do | ||
expect(Role.moderators).to_not include(person) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I'm saying that these tests are for the scope moderators
and it's good to have them, but I'd also like to see them for Role.moderator?(person)
, like we have specs for Role.is_admin?(person)
below.
…r does and correct test syntax diaspora#5324
and refactor coding style according to pull request comments (diaspora#5324)
61f048a
to
81d82dc
Compare
@jhass should be ready now. We rebased to the current develop state. |
Nice! Thank you! |
@Flaburgan sure. How do I indicate that the feature will only be available with the next release? |
Usually we do wiki updates only with the releases. |
The people who are assigned the moderator role should be able to see, mark and delete reported comments/posts. They also should get an email when a new post/comment is reported. Mostly everything works fine, only the email's language doesn't change according to the person's language settings (it takes the language settings of the first one).