-
Notifications
You must be signed in to change notification settings - Fork 462
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-6688 Make accidental history deletions less likely #4759
base: master
Are you sure you want to change the base?
Conversation
@@ -50,6 +50,9 @@ def clear | |||
redirect_to user_readings_path(current_user) | |||
end | |||
|
|||
def confirm_clear |
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 think the tests are failing in part because you need to add this to config/routes.rb as well. After that, you'll probably still need to update the tests to take into account the additional steps of clicking the "Yes, Clear Entire History" button on the new page.
Hey, I'm trying to resolve these failing tests, but I think I'm mostly just digging a deeper hole into things I don't understand. Would I be able to get some guidance, please? |
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 think my comments should address the test failures.
<h2 class="heading"><%= t("Clear Entire History") %></h2> | ||
|
||
<!--main content--> | ||
<%= form_for(@readings, html: { method: "post", action: "clear", class: "simple destroy" }) do |f| %> |
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 form_for
doesn't like getting passed a collection of records. Based on the Rails guide, I think we should be using form_with
in combination with a symbol instead. Solely based on the docs, untested:
<%= form_for(@readings, html: { method: "post", action: "clear", class: "simple destroy" }) do |f| %> | |
<%= form_with(model: :reading, method: :post, url: { action: "clear" }, class: "simple destroy") do |f| %> |
@@ -1,29 +1,29 @@ | |||
<!--Descriptive page name, messages and instructions--> | |||
<h2 class="heading"><%= ts('History') %></h2> | |||
<h2 class="heading"><%= t("History") %></h2> |
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.
Internationalizing a page is not as simple as only replacing ts
with t
. The gist is that you need to use a locale key inside t()
and then place the English text into the locale file.
<h2 class="heading"><%= t("History") %></h2> | |
<h2 class="heading"><%= t(".page_heading") %></h2> |
And in config/locales/views/en.yml
:
readings:
index:
page_heading: History
See the wiki page for I18n for more in-depth information and examples.
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-6688
Purpose
What does this PR do?
Adds a confirmation page for history deletion instead of an alert pop-up.
Updates language from "Clear History" to "Clear Entire History".
Testing Instructions
Credit
calm (they/them)