AO3-6956 Corrected error message for duplicate comments #5135
Conversation
There was a problem hiding this comment.
Hi, Nick!
Thank you so much for this pull request. I reviewed it with a suggestion for how to i18n the error message. Could you please also add a rspec test for the error message?
I've updated the Jira issue status to In Review, so no one will mistakenly create a duplicate pull request. If you'd like the ability to comment on, assign, and transition issues in the future, you're welcome to create a Jira account! You can just reply here with the account name and we'll set up the permissions for you. (It makes things a bit easier for us on the organizational side if the Full Name on your Jira account either closely matches the name you'd like us to credit in the release notes or includes it in parentheses, e.g. "Nickname (CREDIT NAME).")
Thanks again for contributing! If you have any questions, you can contact us at [email protected].
| @@ -71,7 +71,7 @@ def check_for_spam | |||
| validates :comment_content, uniqueness: { | |||
There was a problem hiding this comment.
You can ignore rubocop's Rails/UniqueValidationWithoutIndex complaint, we don't need the unique index for now.
github-actions
bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
|
Still need to fix the test. Don't worry about this right now. |
|
I hard coded the test case in the spec, which is obviously not ideal. A similar method seems to have been done in the spec for kudos though, which is what I'm basing my decision to do the same in my spec on. I can't for the life of me get I18n to recognize the translation in the context of rspec, despite pouring over a lot of documentation. Either way, thank you so much for your suggestions and my username on Jira is Nick Raal. |
Co-authored-by: Bilka <[email protected]>
github-actions
bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
spec/models/comment_spec.rb
Outdated
| expect(second_comment.valid?).to be_falsy | ||
| expect(second_comment.errors.attribute_names).to include(:comment_content) | ||
| expect(second_comment.errors.full_messages).to include("You've already left this comment here. (It may not appear right away for performance reasons.)") |
There was a problem hiding this comment.
I swear I tested my suggestion locally, sorry about all the annoyances for your first PR 😭 The error is correct when displayed, so just do a partial match here in the test instead of a direct equal
* AO3-6956 Corrected error message for duplicate comments * Updated to match suggestions * Added test case * Aligned with formatting guidelines * Fixed test * Update config/locales/models/en.yml Co-authored-by: Bilka <[email protected]> * Updated rspec test to only use partial matching * Normalized translations --------- 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-6956
Purpose
This pull request changes the error message upon posting a duplicate comment from "This comment has already been left on this work. (It may not appear right away for performance reasons." to the more descriptive (and accurate) "You’ve already left this comment here. (It may not appear right away for performance reasons.)"
Credit
niic (he/him)