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

AO3-5860 Prevent leaving comments or kudos when logged in as admin #4378

Merged
merged 47 commits into from
May 17, 2024

Conversation

ceithir
Copy link
Contributor

@ceithir ceithir commented Nov 9, 2022

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-5860

Purpose

Enjoin admins to log out before leaving a comment or kudo.

More exactly:
Hide the relevant buttons as long as they're logged as admin
Return an error message in case such a request reaches the back regardless

Testing Instructions

See JIRA issues (including comments).

Credit

Ceithir (he/him)

@ceithir
Copy link
Contributor Author

ceithir commented Nov 9, 2022

As of now, I've restricted admins only from commenting on works. Not sure if this should be extended to some other types of commentable (chapters?).

@sarken
Copy link
Collaborator

sarken commented Nov 17, 2022

Thanks for working on this! And my apologies; I thought I had replied to this days ago.

It looks like the issue wasn't entirely clear about the scope -- admins shouldn't be able to leave comments or kudos on anything, period. (Volunteers are expected to use user accounts with the official role for communication with users.) The comment form should also be hidden, not just the button.

@sarken sarken changed the title AO3-5860 commenting or leaving kudos with admin accounts AO3-5860 Prevent leaving comments or kudos when logged in as admin Nov 17, 2022
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
spec/controllers/comments_controller_spec.rb Outdated Show resolved Hide resolved
@ceithir
Copy link
Contributor Author

ceithir commented Jun 30, 2023

Conflict with #4492 technically solved. Unsure if it also implies some behavior changes here however.

@sarken
Copy link
Collaborator

sarken commented Jun 30, 2023

I can't think of any changes we'd need off the top of my head!

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

All of the code looks good, but this has merge conflicts with master that need to be solved.

In for a penny
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

In for a penny, in for a pound, indeed.

config/locales/views/en.yml Outdated Show resolved Hide resolved
@@ -51,8 +51,8 @@
(<%= allowed_html_instructions %>)
</h4>
<% else %>
<h4 class="heading"><%= ts("Comment as") %> <span class="byline"><%= current_user.default_pseud.name %></span>
<%= f.hidden_field :pseud_id, :value => "#{current_user.default_pseud.id}", :id => "comment_pseud_id_for_#{commentable.id}" %>
<h4 class="heading"><%= t(".comment_as") %> <span class="byline"><%= current_user.default_pseud.name %></span>
Copy link
Contributor

@Bilka2 Bilka2 Apr 11, 2024

Choose a reason for hiding this comment

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

Not sure on this, but making a note anyway:
Normally we prefer putting variables into the translation string with interpolation instead concatenating in the view files. But that would make the translation not reusable for the multiple pseud case and the formatting looks it'd be a bit awkward to fit in. So I think this is fine to keep like this.

app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Just two more problems left. Thank you for making this form i18n ready!

<% else %>
<span><%= ts("to") %> <%= get_commenter_pseud_or_name(commentable) %> <%= ts("on") %> <%= commentable_description_link(commentable) %></span>
<% end %>
<span><%= t ".inbox_reference",
Copy link
Contributor

Choose a reason for hiding this comment

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

(In both spots) this translation key needs to be ".inbox_reference_html" so that the html of the links is not escaped. According to the i18n wiki page, the t function call should use parens ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the i18n wiki page, the t function call should use parens ().

Might be good to add it to Rubocop then? I'm not sure if there's an elegant way to force https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/MethodCallWithArgsParentheses for a single specific function.

app/views/comments/_comment_form.html.erb Outdated Show resolved Hide resolved
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants