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 Update tests for error message when commenting as admin #4813

Merged
merged 5 commits into from
May 19, 2024

Conversation

ceithir
Copy link
Contributor

@ceithir ceithir commented May 17, 2024

Do the admin check first

Pull Request Checklist

Issue

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

Purpose

Follow-up of #4378
Fix conflict with #4556

I'm actually not sure this is the best solution. Moving first the admin restriction is far more consistent as admins can never comment whatever the circumstances. But I suspect some admins might have a workflow where they check for messages like "you can't add or edit comments on a hidden work". -> Yeah, too many side effects.

For now, I've just updated the tests to match the current behavior. If that behavior is not the intended one, I'm not against a ticket detailing what is expected especially as it has been a long time since my last foray into this.

@@ -79,7 +79,11 @@
<%= flash_div :comment_error, :comment_notice %>

<% commentable_parent = find_parent(commentable) %>
<% if AdminSetting.current.guest_comments_off? && guest? %>
<% if logged_in_as_admin? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of moving this above the other checks, do you think it would make sense to adjust the anon commenting checks to check for admin login instead? Specifically I am thinking that the two !logged_in? in this elsif chain could be guest? instead, because that also checks the admin login.

The reasoning for my idea is that I imagine that admins would like to be able to see the effect of the comment setting on the work without always getting the "please log out" message. But this is just an idea, I am not sure what the best solution is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After sleeping on it, I would be in favor of a quick Scope: Tests Only PR right now to fix the build, and then an addendum to AO3-5860 or a new ticket altogether to rework the behavior (if need be).

Let's try again with less behavior changes
@github-actions github-actions bot added the Scope: Tests Only Only changes automated tests or test configuration label May 17, 2024
@ceithir ceithir marked this pull request as ready for review May 17, 2024 21:15
@sarken sarken merged commit 135caeb into otwcode:master May 19, 2024
26 checks passed
@sarken sarken changed the title AO3-5860 Error message when commenting as admin AO3-5860 Update tests for error message when commenting as admin May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed: Ready to Merge Scope: Tests Only Only changes automated tests or test configuration
Projects
None yet
3 participants