Closed (fixed)
Project:
Legal
Version:
3.0.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
12 Jan 2024 at 15:15 UTC
Updated:
29 May 2024 at 20:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
3liComment #3
luqmaan.essop commentedPatch #2 works well.
Matching string check allows the removal of the duplication notification messages.
Comment #4
anybodyI can confirm this, coming here as I was also searching for the reason for these weird duplicate messages.
These are the two messages shown:
grepping for them returns:
This message appears twice after clicking the one time login link after registration and as it seems, that's even a different message?
Grepping for the message from the patch ("You have just used your one-time login link. It is no longer necessary to use this link to log in. Please set your password.") in core (10.3.x) returns no more results!
So it seems this patch needs to be updated again?
@3li could you check that please?
Here's the relevant line of code in core:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...
And indeed it was changed 8 months ago:
https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/user...
Comment #6
anybodyI prepared an updated MR.
Comment #7
thomas.frobieterWorks for me!
Comment #8
anybodyStill MAJOR as it affects all modern installations and all their registered users.
Comment #9
anybodyMR!16 as static patch attached. Hopefully the maintainer will fix this major issue soon ... :/
Comment #11
anybodyOkay after testing the MR!16 for a few days in production, we received feedback that the UX is still bad.
The reason is, that even after changing the password (after reset) the message is still shown.
This is because
is always present in the URL even after changing the password. Feel free to try yourself. The URL parameters are kept in Drupal 8+ after form submit!
Looking at the blame you can see that this is legacy code from the Drupal 7 port and I'm pretty sure it should be removed, as core already handles everything we need.
That also safes us from having to update the hard-coded string from MR!16 / core again and again!
For that reason I'd suggest to hide MR!16 and instead merge MR!17
Comment #12
anybodyBack to needs review for the new approach. @Grevil will take a look tomorrow and try to align the tests accordingly.
Comment #13
anybodyComment #14
anybodyComment #16
grevil commentedChanges are looking good and everything is working as expected! I'll provide a test to avoid regression in the future.
Comment #17
grevil commentedOk tests need a bit of refactoring. Since the user in the tests never accepted the legal terms, they will appear, once we try to reset our password. BUT this leads to the message not even appearing on the reset password page, because the "password-reset-token" is removed. (That's also why resetting a password without having the legal terms accepted before, does not currently work. See #3074688: Password can not be reset, when user hasn't accepted the legal terms yet).
Comment #18
grevil commentedAlright, finished refactoring the tests! Please review!
Comment #20
anybodyGreat work @Grevil! Thank you for reworking the tests and adding GitLab CI to show it works as expected now!
RTBC!
Comment #21
robert castelo commentedMessage is not shown at all when user needs to accept T&C.
Steps:
1. Create new T&C version
2. Use one time login link
3. User is asked to accept T&C - accept
4. User is shown password reset page - no message is shown
Comment #22
grevil commented@Robert Castelo, my apologies, we should have properly adjusted the steps to reproduce before waiting for your review. The user needs to have already accepted the T&C, before resetting their password.
Here are the steps to reproduce (I will also add them in the issue summary):
The reset notice showing up twice:

The reset notice still showing once, after defining a new password:

I hope that helps!
Comment #23
anybody@Grevil: Thanks! We should also check the case @Robert Castelo described and have tests for all possible cases, so we can be sure
Both should be checked in the tests.
As a first step the case described by @Robert Castelo should also be checked manually.
Comment #24
grevil commentedThat are exactly the tests that we implemented here. ;)
One is currently commented out and will be implemented in the follow-up issue.
Comment #26
grevil commentedThanks, @Robert Castelo! Don't forget to credit all people involved!
Comment #27
grevil commentedJust a tiny heads up, the phpunit pipeline currently fails: https://git.drupalcode.org/project/legal/-/jobs/1593395
But not related to any tests done here, just some legacy tests defining a deprecated theme as their test theme.
Comment #28
grevil commentedThis issue caused regression: #3447367: The one-time login message is not displayed anymore, when the user is first redirected to the T&C page. (Although minor).
Comment #29
robert castelo commentedRegression issue fixed.