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 password confirm validation error message #11625

Merged

Conversation

greenwoodt
Copy link
Contributor

@greenwoodt greenwoodt commented Sep 20, 2023

🎩 What? Why?

This is a password confirmation alert message change to allow the alert to be more clearer according to the original issuer #11283.

An original PR to this issue was moved from 0.28 to 0.27: #11534

📌 Related Issues

Link your PR to an issue

Testing

  1. Head over to Sign in/Log in
  2. Click create an account
  3. Fill the form
  4. Purposely create different passwords in the confirmation field and hit enter
  5. See the the error output has quotes

📷 Screenshots

♥️ Thank you!

@greenwoodt greenwoodt added module: core release: v0.27 Issues or PRs that need to be tackled for v0.27 labels Sep 21, 2023
@greenwoodt greenwoodt self-assigned this Sep 21, 2023
@greenwoodt greenwoodt added the type: fix PRs that implement a fix for a bug label Sep 21, 2023
@greenwoodt
Copy link
Contributor Author

I have updated changes to allow the error message to appear both as a user that has logged in and wishes to change their password and sign up from the perspective of a new user however, the error message repeats itself twice in decidim-core/app/forms/decidim/registration_form.rbas seen in the sc below:

Screenshot 2023-09-26 at 15 27 27

@greenwoodt greenwoodt marked this pull request as ready for review September 26, 2023 13:52
@andreslucena andreslucena changed the title Password confirm validation Fix password confirm validation error message Sep 27, 2023
@andreslucena
Copy link
Member

I have updated changes to allow the error message to appear both as a user that has logged in and wishes to change their password and sign up from the perspective of a new user however, the error message repeats itself twice in decidim-core/app/forms/decidim/registration_form.rbas seen in the sc below:

@greenwoodt I check out this error and I saw that this is actually done because we're using full_messages and adding the error message in the flash alert. For what I could see, this is something that we're only doing in this form, so for consistency we should leave the generic error message in the flash alert. Mind that the validation error is shown in the form itself:

Screenshot of the registration form with the generic error message
Screenshot of the field with the error message

For changing this, you need to change the flash.now[:alert] in decidim-core/app/controllers/decidim/devise/registrations_controller.rb. This is what I have in mind:

diff --git a/decidim-core/app/controllers/decidim/devise/registrations_controller.rb b/decidim-core/app/controllers/decidim/devise/registrations_controller.rb
index ba62d5330b..17744906f9 100644
--- a/decidim-core/app/controllers/decidim/devise/registrations_controller.rb
+++ b/decidim-core/app/controllers/decidim/devise/registrations_controller.rb
@@ -39,7 +39,7 @@ module Decidim
           end
 
           on(:invalid) do
-            flash.now[:alert] = @form.errors.full_messages.join(", ") if @form.errors.full_messages.any?
+            flash.now[:alert] = t("error", scope: "decidim.devise.registrations.create")
             render :new
           end
         end
diff --git a/decidim-core/config/locales/en.yml b/decidim-core/config/locales/en.yml
index 25a425f31b..3ad9735657 100644
--- a/decidim-core/config/locales/en.yml
+++ b/decidim-core/config/locales/en.yml
@@ -534,6 +534,8 @@ en:
           subtitle: Please fill in the following form in order to complete the sign up
           username_help: Public name that appears on your posts. With the aim of guaranteeing the anonymity, can be any name.
       registrations:
+        create:
+          error: There was a problem creating your account.
         new:
           already_have_an_account?: Already have an account?
           newsletter: Receive an occasional newsletter with relevant information

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I have a couple suggestions and also a change in the flash alert. Can you check it out please?

decidim-core/spec/forms/account_form_spec.rb Outdated Show resolved Hide resolved
decidim-core/spec/forms/account_form_spec.rb Outdated Show resolved Hide resolved
greenwoodt and others added 3 commits September 27, 2023 12:04
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

Functionality checked, and works as advertised.

@greenwoodt, can you have a look on failing specs?

@alecslupu alecslupu added this to the 0.27.5 milestone Oct 5, 2023
@greenwoodt
Copy link
Contributor Author

greenwoodt commented Oct 10, 2023

@alecslupu and @andreslucena Apologies for taking some time ironing out the tests on this PR but upon closer inspection the release/0.27-stable branch is actually broken with a Catalan translation to "Sign in" on the Core (system specs) / Tests this should be fixed with commit: 2a37840

Screenshot 2023-10-10 at 10 32 44

@greenwoodt
Copy link
Contributor Author

greenwoodt commented Oct 10, 2023

Changes have made also been made to the flash error message to simplify the Rspec test in the decidim-core/spec/controllers/registrations_controller_spec.rb (requested by @andreslucena)

Feel free to review anytime @alecslupu

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Just a small change and we're good to go

decidim-core/config/locales/ca.yml Show resolved Hide resolved
decidim-core/config/locales/ca.yml Outdated Show resolved Hide resolved
@andreslucena
Copy link
Member

Also, as we're changing how we're handling the errors in this form, I think we should also change how this flash alert works in develop, just to keep it consistent within versions

flash.now[:alert] = @form.errors.full_messages.join(", ") if @form.errors.full_messages.any?

We can do it once this is merged

@greenwoodt
Copy link
Contributor Author

greenwoodt commented Oct 10, 2023

Also, as we're changing how we're handling the errors in this form, I think we should also change how this flash alert works in develop, just to keep it consistent within versions

flash.now[:alert] = @form.errors.full_messages.join(", ") if @form.errors.full_messages.any?

We can do it once this is merged

Ok!

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

LGTM! As I was involved in the code, I'll ask for @alecslupu review before merging

Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

LGTM
image

@alecslupu alecslupu merged commit 01dfd49 into decidim:release/0.27-stable Oct 19, 2023
85 of 86 checks passed
andreslucena added a commit that referenced this pull request Nov 3, 2023
* Password confirm validation

* changes to password confirm message

* pushing whats been changed

* updated changes to add quotes to error message on pw

* normalisation of i18n strings in locales

* Update decidim-core/spec/forms/account_form_spec.rb

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

* Added test in registeration_form_spec and change in flash.now[:alert] registrations controller

* test failure changes

* Fixing of SPEC failures

* rspec tests fix

* test

* Broken test with Catalan translation  for sign-in fix

* Registration form spec fix

* fixes to rspec registerations controller on flash message

* Update decidim-core/config/locales/ca.yml

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

---------

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core release: v0.27 Issues or PRs that need to be tackled for v0.27 type: fix PRs that implement a fix for a bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants