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

fix for user edit bug => Issue #58 #79

Merged
merged 3 commits into from Oct 5, 2021
Merged

Conversation

Xelaflash
Copy link
Contributor

What this Pull Request do ?

Fixing the edit user bug (issue #58)
Fix:
• Some mandatory Devise config for strong params where missing in app/controllers/application_controllers.rb
• In app/views/devise/registrations/edit.html.erb: the default fields (passwordpassword_confirmation and current_password) were not present in the form preventing the save action for User#edit to pass.
(note: current password is mandatory for Devise#edit action)

Close an issue ? please insert the hash

Close #58

Required migrations ?

[] yes [x] no

Add a new gem ?

[] yes [x] no

Add a new package ?

[] yes [x] no

@vczb
Copy link
Owner

vczb commented Oct 5, 2021

@Xelaflash Hello, thank you for your pull request. We are using prettier to format the files, can you run yarn prettier:format and submit the PR again please ? this is necessary to approve on CI

Copy link
Owner

@vczb vczb left a comment

Choose a reason for hiding this comment

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

Very nice, just a I18n key missing, waiting to the fix.

required: true,
label_html: { class: "form-label label-small" },
input_html: { autocomplete: "current-password", class: 'form-text' } %>
<i class="form-label label-small my-1">(we need your current password to confirm your changes)</i>
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please add this text to I18n ?

https://github.com/vczb/gamou/tree/main/config/locales

create a new key on config/locales/en/en.yml and add this text like this

I18n.t('my.new.key')

its necessary replicate add this content on pt-BR locale too config/locales/pt-BR/pt-BR.yml with te content

"Precisamos de sua senha atual para confirmar suas alterações"

If you need help im available

@vczb vczb temporarily deployed to gamou-pr-79 October 5, 2021 18:50 Inactive
@vczb
Copy link
Owner

vczb commented Oct 5, 2021

@Xelaflash i don't understand why need the password three times.

Captura de tela de 2021-10-05 15-57-26

What you think about the password confirmation is the last information on the form ? just one time only, use the * CURRENT PASSWORD

Captura de tela de 2021-10-05 16-00-49

@Xelaflash
Copy link
Contributor Author

Hi,
i'll handle the prettier and the i48n (sorry did pay attention to internationalisation part).

For the password fields, they are part of Devise default views and also default strong params.
We could always remove them as they are not required but as for UX i believe that as a user i should be able to modify my password in my account page.
But this is totally up to you and we can remove those 2 fields and put the current_password (which is mandatory) at the end of the form.

Just confirm what you decide.

@vczb
Copy link
Owner

vczb commented Oct 5, 2021

Hi, i'll handle the prettier and the i48n (sorry did pay attention to internationalisation part).

For the password fields, they are part of Devise default views and also default strong params. We could always remove them as they are not required but as for UX i believe that as a user i should be able to modify my password in my account page. But this is totally up to you and we can remove those 2 fields and put the current_password (which is mandatory) at the end of the form.

Just confirm what you decide.

ok, good answer, we can keep the 3 password fields, but lets move to the last input of the form ?

@vczb
Copy link
Owner

vczb commented Oct 5, 2021

Hi, i'll handle the prettier and the i48n (sorry did pay attention to internationalisation part).

For the password fields, they are part of Devise default views and also default strong params. We could always remove them as they are not required but as for UX i believe that as a user i should be able to modify my password in my account page. But this is totally up to you and we can remove those 2 fields and put the current_password (which is mandatory) at the end of the form.

Just confirm what you decide.

Or just move the * CURRENT PASSWORD to the down of * DIAMOND PRICE IN CENTS, will be good!


PR preview:

https://gamou-pr-79.herokuapp.com/

…ing text / change in app controller after prettier format
@vczb vczb temporarily deployed to gamou-pr-79 October 5, 2021 19:47 Inactive
@Xelaflash
Copy link
Contributor Author

Changes have been made.

  1. run prettier
  2. change positioning of current_password field
  3. edit en locale for new text (translation where actually un pt-BR file, cause they are devise default, therefore i did not touch the translation, please check if ok on your side)
  4. change a little bit the styling of the devise help text (smaller and lighter)

Screenshot 2021-10-05 at 15 39 04

@vczb vczb temporarily deployed to gamou-pr-79 October 5, 2021 19:52 Inactive
@vczb
Copy link
Owner

vczb commented Oct 5, 2021

Changes have been made.

  1. run prettier
  2. change positioning of current_password field
  3. edit en locale for new text (translation where actually un pt-BR file, cause they are devise default, therefore i did not touch the translation, please check if ok on your side)
  4. change a little bit the styling of the devise help text (smaller and lighter)

Screenshot 2021-10-05 at 15 39 04

You are the best, thank you!

@vczb vczb merged commit 6ec5161 into vczb:main Oct 5, 2021
vczb pushed a commit that referenced this pull request Oct 6, 2021
…text / change in app controller after prettier format
vczb pushed a commit that referenced this pull request Oct 11, 2021
…text / change in app controller after prettier format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Can't edit user
2 participants