-
Notifications
You must be signed in to change notification settings - Fork 491
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-6308 fix user suspension end time #4338
base: master
Are you sure you want to change the base?
AO3-6308 fix user suspension end time #4338
Conversation
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.
It could be helpful to extend any tests we have that involve this function to include one where the date changes because of the threshold. I think we already have some unit tests for banning, but maybe not in an application_controller_spec.rb
# If the stated suspension end date is after the unban threshold we need to advance a day | ||
suspension_end = suspension_end.next_day(1) if suspension_end > unban_theshold | ||
localized_suspension_end = DateTime.parse(localize(suspension_end)).strftime("%Y-%m-%d") | ||
flash[:error] = t("suspension_notice", default: "Your account has been suspended until %{suspended_until}. You may not add or edit content until your suspension has been resolved. Please <a href=\"#{new_abuse_report_path}\">contact Abuse</a> for more information.", suspended_until: localized_suspension_end).html_safe |
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.
We're trying to avoid HTML in translation strings when possible; could you update the string here to use a variable (like "%{abuse_link}"
) and the link_to
method? The code in https://github.com/otwcode/otwarchive/blob/master/app/controllers/users/passwords_controller.rb#L11 does something similar
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.
I've edited line 441 to be like so
flash[:error] = t(".users.status.suspension_notice", suspended_until: localized_suspension_end, contact_abuse_link: view_context.link_to(t(".contact_abuse"), new_abuse_report_path)).html_safe
and added the following to en.yml:
status:
ban_notice: Your account has been banned. You are not permitted to add or edit archive content. Please %{contact_abuse_link} for more information.
suspension_notice: Your account has been suspended until %{suspended_until}. You may not add or edit content until your suspension has been resolved. Please %{contact_abuse_link} for more information.
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.
However I still get missing translations and unused key errors, so something is wrong with the link between the controller and the translation file. Do you know what I'm doing wrong?
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.
It looks like the test is expecting the following absolute/fully-qualified keys
+--------+----------------------------------------------------+--------------------------------------------------------+
| Locale | Key | Value in other locales or source |
+--------+----------------------------------------------------+--------------------------------------------------------+
| en | application.check_user_not_suspended.contact_abuse | app/controllers/application_controller.rb:423 |
| en | application.check_user_status.contact_abuse | app/controllers/application_controller.rb:401 (1 more) |
+--------+----------------------------------------------------+--------------------------------------------------------+
If you wanted to use a key like
contact_abuse: contact Policy & Abuse |
users.passwords.create.contact_abuse
) -- although I would recommend extracting that key to a common location if it's used in multiple controllers
It would be helpful to add a test? |
# If the stated suspension end date is after the unban threshold we need to advance a day | ||
suspension_end = suspension_end.next_day(1) if suspension_end > unban_theshold | ||
localized_suspension_end = localize(suspension_end.to_date) | ||
flash[:error] = t("users.status.suspension_notice", suspended_until: localized_suspension_end, contact_abuse_link: view_context.link_to(t("users.contact_abuse"), new_abuse_report_path)).html_safe |
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.
I would be nice to have the key end in _html
so that the html_safe call isn't needed:
flash[:error] = t("users.status.suspension_notice", suspended_until: localized_suspension_end, contact_abuse_link: view_context.link_to(t("users.contact_abuse"), new_abuse_report_path)).html_safe | |
flash[:error] = t("users.status.suspension_notice_html", suspended_until: localized_suspension_end, contact_abuse_link: view_context.link_to(t("users.contact_abuse"), new_abuse_report_path)) |
This also needs a change in the locale file. Same applies to the other t() calls.
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.
Update: This change was done in 61a4f52 with some other work. So you should merge the master
branch into your branch to get the updated locale key and locale files.
Actions identified by weeklies and Bilka2
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.
Please add a test, thank you!
Is there already a _spec file defined that tests things in application_controller.rb? I can't seem to find one. |
If I edit the file user_manager.rb and change the function suspend_user like so
then I know how to test this change, but did you want the fix to the user suspension end time to be a thing that is only shown to users? |
The fix should only be in the message for the user, as it is right now. You can test the user message by making a cucumber scenario where a suspended user logs in and tries to access the "new work" page. They should get redirected to their dashboard and see the flash message about being suspended. It will probably be helpful to have a known current date and time in the test by using a step like |
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.
Thank you very much!
@@ -14,14 +14,14 @@ def create | |||
end | |||
|
|||
if user.prevent_password_resets? | |||
flash[:error] = t(".reset_blocked", contact_abuse_link: view_context.link_to(t(".contact_abuse"), new_abuse_report_path)).html_safe | |||
flash[:error] = t(".reset_blocked_html", contact_abuse_link: view_context.link_to(t("users.contact_abuse"), new_abuse_report_path)) |
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.
Apologies for coming back to this after already approving, but I had some discussion with the Translation team:
It would be nice if reset_blocked_html
would continue to use a different locale key for the policy and abuse link text than the status texts. That allows translators to translate the link differently in context, since the sentence in reset_blocked
is different than the one used in suspension_notice_html
and ban_notice_html
.
flash[:error] = t(".reset_blocked_html", contact_abuse_link: view_context.link_to(t("users.contact_abuse"), new_abuse_report_path)) | |
flash[:error] = t(".reset_blocked_html", contact_abuse_link: view_context.link_to(t(".contact_abuse"), new_abuse_report_path)) |
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.
done
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.
Thank you for bearing with me!
I left some more code suggestions, but they're completely optional because they only change the formatting of the dates. So I already flipped the PR label to ready to merge.
Co-authored-by: Bilka <[email protected]>
Co-authored-by: Bilka <[email protected]>
Co-authored-by: Bilka <[email protected]>
Co-authored-by: Bilka <[email protected]>
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing
)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-6308
Purpose
What does this PR do?
Fix incorrect time for the end of a user's suspension
Testing Instructions
How can the Archive's QA team verify that this is working as you intended?
If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.
References
Are there other relevant issues/pull requests/mailing list discussions?
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
Cesium-Ice, they/them
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.
Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.