[op-18418] Sign up errors are showing behind new account creation modal by bsatarnejad · Pull Request #24306 · opf/openproject

bsatarnejad · 2026-07-16T13:26:36Z

Ticket

https://community.openproject.org/wp/OP-18418

Screenshots

What approach did you choose and why?

Primerize the cerate-new-account page

github-actions · 2026-07-16T13:56:07Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]
rspec ./spec/features/roles/report_spec.rb[1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24306, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`
- `rspec ./spec/features/roles/report_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #24306. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24306 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @bsatarnejad to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @bsatarnejad, and request a review from @bsatarnejad.
On every commit, set @bsatarnejad as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

HDinger

This might work, but feels somehow cumbersome to remember the error by hand and then render it in the modal. Further, it will be obsolete as soon as we migrate the page to Primer.. Maybe I am missing something here, but I see no reason that this needs to be in Angular. Also, I don't think that this have to be a dialog.
Imho, it would be best to migrate the form to a primer form, get rid of the dialog and just render the form. We can still center the form if desired.

registration modal instead of using the page-level flash area.

github-actions · 2026-07-20T11:09:35Z

Warning

Flaky specs

rspec ./modules/team_planner/spec/features/query_handling_spec.rb[1:3:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24306, linked for reference only):

- `rspec ./modules/team_planner/spec/features/query_handling_spec.rb[1:3:1:1]`

Treat this as a standalone task, unrelated to PR #24306. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24306 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @bsatarnejad to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @bsatarnejad, and request a review from @bsatarnejad.
On every commit, set @bsatarnejad as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

github-actions · 2026-07-21T06:18:34Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #24306, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`

Treat this as a standalone task, unrelated to PR #24306. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24306 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @bsatarnejad to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @bsatarnejad, and request a review from @bsatarnejad.
On every commit, set @bsatarnejad as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

HDinger

I think the error handling is the biggest issue here at the moment.. It feels like you are overcomplicating it a bit and in the final result is somehow confusing in code as well as in the final UI.

HDinger · 2026-07-21T07:11:48Z

+class Account::LostPasswordForm < ApplicationForm
+  form do |form|
+    form.text_field(
+      name: :mail,


Suggested change

name: :mail,

name: :mail,

type: :email

HDinger · 2026-07-21T07:14:05Z

+        label: User.human_attribute_name(:new_password),
+        type: :password,
+        required: true,
+        autocomplete: "off",


Suggested change

autocomplete: "off",

autocomplete: "new-password",

HDinger · 2026-07-21T07:14:27Z

+        label: User.human_attribute_name(:password_confirmation),
+        type: :password,
+        required: true,
+        autocomplete: "off",


Suggested change

autocomplete: "off",

autocomplete: "current-password",

I was intruiged to learn about these fancy new autocomplete attributes ("new" refers to "new to me").

However, I found it confusing that the password recovery form would have autocompletion for the "current password". Are you sure this is correct?

According to MDN (I didn't dig deeper than that):

new-password

A new password. When creating a new account or changing passwords, this should be used for an "Enter your new password" or "Confirm new password" field, as opposed to a general "Enter your current password" field that might be present. This may be used by the browser both to avoid accidentally filling in an existing password and to offer assistance in creating a secure password.

current-password

The user's current password.

HDinger · 2026-07-21T07:15:34Z

@@ -0,0 +1,13 @@
+# frozen_string_literal: true


The copyright header is missing

HDinger · 2026-07-21T07:15:39Z

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+


The copyright header is missing

HDinger · 2026-07-21T09:04:18Z

 ++#%>
 <% html_title t(:label_password_lost) %>

+<%= error_messages_for @user if @user&.errors&.any? %>


Instead of rendering the errors manually, you sould show them inline in the form. Therefore you need to set the model to the form which should be initialized on GET and not only when there is an error

HDinger · 2026-07-21T09:07:13Z

+      helpers.format_text(helpers.user_consent_instructions(I18n.locale), target: "_blank")
+    end
+    form.check_box(name: :consent_check,
+                   scope_name_to_model: false,


Why is scope to model false? If i submit a form with errors, the checkbox is not checked any more as it has lost the connection to the model.

HDinger · 2026-07-21T09:11:40Z

+  <%= error_messages_for :user %>
+
+  <%= primer_form_with(
+        model: @user,
+        url: account_register_path,
+        id: "new_user",
+        data: { turbo: false, test_selector: "registration-form" }


Similar issue here: You passed the model, so the errors appear inline, but since you also render the errors by hand, they appear twice:

HDinger · 2026-07-21T09:15:02Z

      invite: Meghívás
      message:
-        description: Küldünk egy e-mailt a %{principal} címre, amelyhez itt hozzáadhat egy személyes üzenetet. Hasznos lehet egy magyarázat a meghívóhoz, vagy esetleg egy kis információ a projekttel kapcsolatban, hogy segítsen az elindulásban.
+        description: Küldünk egy e-mailt a felhasználónak, amelyhez itt hozzáadhat egy személyes üzenetet. Hasznos lehet egy magyarázat a meghívóhoz, vagy esetleg egy kis információ a projekttel kapcsolatban, hogy segítsen az elindulásban.


Crowdin strings should never be fixed in code as they get overwritten with the next update. The issue has been fixed by now on the base branch. So I suggest you revert this commit

HDinger · 2026-07-21T09:19:55Z

+    return true unless user_consent_required?
+    return true if consent_param?
+
+    user.errors.add(:base, I18n.t("consent.failure_message"))


Why do you add the error to base and not to the field directly?

HDinger · 2026-07-21T09:28:40Z

Since this has bigger dimension by now, I'd suggest we move it 17.8. Wdyt?

bsatarnejad · 2026-07-21T10:43:24Z

Since this has bigger dimension by now, I'd suggest we move it 17.8. Wdyt?

Agree, it is more than a bug fix effort.

…ehind-new-account-creation-modal

NobodysNightmare · 2026-07-21T13:40:54Z

 wclass = local_assigns[:wide] ? "wide" : ""
 %>
-<% if auth_provider_html.strip != '' %>
+<% if auth_provider_html.to_s.match?(/auth-provider/) %>


This is very surprising. I guess this is the fix/workaround for showing the "or login via" line even when no providers are available.

Do you have an explanation for why this new check works and the old one didn't? It looks even more like a hack now than it looked before 🙈

bsatarnejad added bugfix needs review labels Jul 16, 2026

bsatarnejad self-assigned this Jul 16, 2026

bsatarnejad marked this pull request as ready for review July 16, 2026 13:56

HDinger requested changes Jul 17, 2026

View reviewed changes

bsatarnejad force-pushed the op-18418-signup-errors-are-showing-behind-new-account-creation-modal branch from 65e3882 to 86f4697 Compare July 20, 2026 07:26

bsatarnejad added 7 commits July 20, 2026 10:05

Render account registration validation and activation errors inside the

3c5a9fe

registration modal instead of using the page-level flash area.

Undo changes in error helper

4868bc5

Primerize the registration form

dd37951

fix yml errors

5958e3e

fix failng feature tests

487cf12

Primerize lost password form

5a3043a

Fix failing tests

f621a0b

bsatarnejad force-pushed the op-18418-signup-errors-are-showing-behind-new-account-creation-modal branch from 83fca9f to f621a0b Compare July 20, 2026 08:05

bsatarnejad added 4 commits July 20, 2026 10:28

Fix failing tests

941fccc

Fix failing tests

1629ce7

Remove breadcrumbs for lost password page

d65e86c

Remove breadcrumbs for lost password page

870fa78

Move consent_given_for_registration? to the helper

8dd491b

bsatarnejad requested review from HDinger and NobodysNightmare July 21, 2026 06:22

HDinger requested changes Jul 21, 2026

View reviewed changes

Merge branch 'release/17.7' into op-18418-signup-errors-are-showing-b…

ba76862

…ehind-new-account-creation-modal

NobodysNightmare reviewed Jul 21, 2026

View reviewed changes

Uh oh!

Conversation

bsatarnejad commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Screenshots

What approach did you choose and why?

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

HDinger left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

HDinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HDinger commented Jul 21, 2026

Uh oh!

bsatarnejad commented Jul 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

bsatarnejad commented Jul 16, 2026 •

edited

Loading