-
Notifications
You must be signed in to change notification settings - Fork 667
AO3-7263 Error 500 when accessing user preferences as an admin for a user that has locale preferences enabled #5544
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
base: master
Are you sure you want to change the base?
Conversation
…e instead of current user
marcus8448
left a comment
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.
Hi varram,
Thanks for the pull request! It would be great if you could also add a test for this issue. (You might find preferences_edit.feature helpful when doing so)
|
hey! @marcus8448 I've made the changes you requested. Let me know if this is fine? (I'm pretty new to contributing to ao3 so any feedback is appreciated :)) |
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.
Implementation looks good! Just two notes for the tests.
|
|
||
| context "when locale preferences are enabled for the viewed user" do | ||
| before do | ||
| allow($rollout).to receive(:active?).with(:set_locale_preference, user).and_return(true) | ||
| end | ||
|
|
||
| read_roles.each do |role| | ||
| it "allows #{role} to view preferences without errors" do | ||
| admin = create(:admin, roles: [role]) | ||
| fake_login_admin(admin) | ||
|
|
||
| get :index, params: { user_id: user.login } | ||
|
|
||
| expect(assigns(:user)).to eq(user) | ||
| expect(assigns(:preference)).to eq(user.preference) | ||
| expect(response).to be_successful | ||
| end | ||
| end | ||
| end |
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.
RSpec doesn't render views by default, so this doesn't actually hit the problematic code in the view.
I think the Cucumber test should be sufficient to cover this issue anyways, so this can just be removed.
| When I go to testuser's preferences page | ||
| Then I should see "Set My Preferences" | ||
| And I should see "Your locale" | ||
| And I should not see "We're sorry, but something went 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.
When the error is raised, the test automatically fails so we don't need to check for the error page text.
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-7263
Purpose
Fixes incorrect reference to
current_user.preference.locale_for_mailsin the preferences view. The view was usingcurrent_user.preferenceinstead of the already-loaded@preferenceinstance variable, which could potentially cause errors and was inconsistent with the rest of the form.Changes:
app/views/preferences/index.html.erbto use@preference.locale_for_mailsinstead ofcurrent_user.preference.locale_for_mails@preference.preferred_localeon line 78)app/views/help/preferences_locale.html.erbTesting Instructions
policy_and_abuse,supportorsuperadminadmina. Browse > Works
b. Follow the link to an author’s pseud
c. Click on preferences in the user sidebar
Credit
varram (he/him)