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

Finalize login page migration #36348

Merged
merged 18 commits into from
Jun 26, 2024

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Jun 11, 2024

Questions Answers
Branch? develop
Description? Finalize login page migration
Type? new feature
Category? BO
BC breaks? yes
Deprecations? no
How to test? CI green UI tests green Functional test: the login page in BO, the reset password feature, any campaign/scenario related to login page
UI Tests https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/9676037962 https://github.com/florine2623/testing_pr/actions/runs/9663231021
Fixed issue or discussion? Fixes #36347
Related PRs ~
Sponsor company ~

boherm
boherm previously approved these changes Jun 12, 2024
@@ -886,41 +886,6 @@
<title>Perform actions before controller initialization</title>
<description>This hook is launched before the initialization of all controllers</description>
</hook>
<hook id="actionAdminLoginControllerBefore">
Copy link
Contributor

Choose a reason for hiding this comment

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

@jolelievre do you plan to reintroduce those hooks? one way or another, I'm pretty sure they've been useful for some 2FA modules

Copy link
Contributor

Choose a reason for hiding this comment

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

@jolelievre unless there are certain events triggered on the SF side of things that could be a replacement

Copy link
Contributor Author

@jolelievre jolelievre Jun 12, 2024

Choose a reason for hiding this comment

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

No these hooks were triggered by the legacy controller, the legacy controller is no more and it wouldn't make sense to maintain them because we can't dispatch the same parameters (which was the legacy controller)

Usually we actually never maintain/remove the hooks removed along with legacy pages, but this time I thought it as worth the extra effort This way I also noted which hooks were actually removed and I'm adding them in the dev doc

My plan is to explain which hook were removed, which ones were maintained (two actually), and what alternative hooks can be used no even if they are different, I'll ping you on the PR for the doc

If we realize some hooks are missing we can also think of what new hooks could/should be introduced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one hook that was kept strictly for legacy retro compatibility here:

$this->hookDispatcher->dispatchWithParameters(
'actionAdminLoginControllerSetMedia',
[
'controller' => $this->legacyControllerContext,
]
);

It does provide a legacy controller, or actually a "fake" legacy controller we implemented as a bridge to still allow some legacy modifications like $legacyController->addJs

Maintaining this hook to allow adding assets was relevant, but all the other hooks would be overkill IMHO It gives the wrong impression that nothing changed and it forces us to maintain retro compatibility in too many places

Progi1984
Progi1984 previously approved these changes Jun 12, 2024
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

Ok for tests/UI directory

@jolelievre jolelievre added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 12, 2024
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Jun 12, 2024
@florine2623 florine2623 self-assigned this Jun 13, 2024
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @jolelievre ,

Tested and compared to old login page.

QA OK ✅

  • Standard employee email/password login
  • Correct employee email/wrong password
  • Non-existing email/any password
  • Logout from BO
  • Links to prestashop-project.org and social medias
  • Make sure the work done by @M0rgan01 for backoffice cron calls work correctly with the migrated Login system (search cron regeneration)
  • Responsive
  • Correct email is sent for Forget my password feature

QA NOK ❌ or needs improvement ⚠️

BUG 1

Changing the name of shop (Contact > Stores) doesn't change the title of the login page

Screenshot 2024-06-13 at 14 06 54

BUG 2

Clickable link "back to FO" is too long. I can click anywhere on the header and it will go back to my FO.
Screenshot 2024-06-13 at 14 00 03

BUG 3

Non-valid email is not detected. I should have a warning message and I should not be able to click on Log In until my email address has a valid format.

It should be like :
Screenshot 2024-06-13 at 14 04 42

It is like so :
Screenshot 2024-06-13 at 14 04 55

BUG 4

When a reset link is sent, the message is different between Old and New Login page.
Old Login Page :
Screenshot 2024-06-13 at 14 22 58
New Login page :
Screenshot 2024-06-13 at 14 28 44

BUG 5

Employees with different roles and permissions cannot access the BO. I created a translator, when I log in with correct email/password, I have an exception. I kept the Default page as Dashboard (not accessible by this employee).
Screenshot 2024-06-13 at 14 29 58

BUG 6

When my login page has been open long enough and refreshed, I have a CSRF alert. Even after refreshing the page the alert is displayed, I have to manually close the alert.
Screenshot 2024-06-13 at 13 53 55

BUG 7

Alert message in Log In page is still displayed after I go to Forget my password page. I should not have to manually close it if I move to another page.

Screen.Recording.2024-06-13.at.14.43.55.mov

BUG 8

maybe not bug but feature, but I couldn't find any info on that topic)
I can't receive multiple "Forget my password" emails. I have to wait 24 hours. This cannot be configured. In Old Login Page I could send as many as I wanted.
Screenshot 2024-06-13 at 14 51 50

BUG 9

In Multistore context, I created a second-store, granted access to only 1 employee Translator. Tried to log in via my translator employee account ❌ tried to access m store via my default SuperAdmin employee ❌
BO and login page can't be accessed anymore.
Screenshot 2024-06-13 at 15 01 25

Waiting for your feedback ^^
Thanks!

@florine2623 florine2623 added the Waiting for author Status: action required, waiting for author feedback label Jun 13, 2024
@jolelievre jolelievre force-pushed the finalize-login-page-migration branch from 138a7c5 to 6753c0b Compare June 17, 2024 17:12
boherm
boherm previously approved these changes Jun 17, 2024
@jolelievre
Copy link
Contributor Author

jolelievre commented Jun 17, 2024

Thanks @florine2623

here are the bugs I fixed with the recent changes:

Bug 1 The form title now uses the global configuration (previously it used the Shop->name which stores the shop name in another place)
Bug 2 Fixed
Bug 3 The new login page now handles the email validation, the UX is not 100% the same because we use a different BO theme and a different version of bootstrap, but it should be iso-functional besides that.
Bug 4 The wording was updated to match the previous one (I had copied some other wording for Ajax return, not adapted in this context indeed)
Bug 5 Actually, the bug was in the employee edition form, when the profile is changed the available options for the home page is supposed to be updated to only propose available values, this feature was broken on develop and is now fixed. If the homepage is edited in some other manual ways (direct DB edit for example) then the access denied is still relevant. However it is handled differently now, we no longer automatically redirect to homepage we display an error page (in prod mode you get a page with the menu so you can navigate to another page).
Bug 7 Fixed, all the alert messages are removed when we click of I forgot my password
Bug 8 Yes my bad I misunderstood the initial feature, the password reset is only blocked temporarily AFTER you actually reset your password, and the delay is configurable in Advanced Parameters > Team > Password regeneration
Bug 9 Fixed

This is the only don't that wasn't fixed, but it matches the usual behaviour of Symfony
Bug 6 This is the native behaviour of the CQRS feature in Symfony, I don't think we should change it, the errors can be closed and on the second try, they should disappear.

@jolelievre jolelievre removed Waiting for author Status: action required, waiting for author feedback Waiting for QA Status: action required, waiting for test feedback labels Jun 17, 2024
@ps-jarvis ps-jarvis added develop Branch Feature Type: New Feature BC break Type: Introduces a backwards-incompatible break labels Jun 17, 2024
boherm
boherm previously approved these changes Jun 18, 2024
boherm
boherm previously approved these changes Jun 19, 2024
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Jun 19, 2024
@jolelievre jolelievre force-pushed the finalize-login-page-migration branch from 35693aa to 5304c0c Compare June 25, 2024 18:30
@jolelievre jolelievre removed the Waiting for QA Status: action required, waiting for test feedback label Jun 25, 2024
@jolelievre jolelievre force-pushed the finalize-login-page-migration branch from 5304c0c to bc58627 Compare June 25, 2024 20:49
@jolelievre jolelievre removed the Waiting for author Status: action required, waiting for author feedback label Jun 26, 2024
boherm
boherm previously approved these changes Jun 26, 2024
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Jun 26, 2024
@jolelievre jolelievre dismissed stale reviews from matthieu-rolland and boherm via 3b8034f June 26, 2024 09:34
@jolelievre jolelievre force-pushed the finalize-login-page-migration branch from 522671e to 3b8034f Compare June 26, 2024 09:34
@jolelievre jolelievre removed the Waiting for QA Status: action required, waiting for test feedback label Jun 26, 2024
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Jun 26, 2024
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @jolelievre ,

BUG 5 ✅

Works like a charm !
The employee has only access to the pages that are accessible by his role !

Screen.Recording.2024-06-26.at.12.08.16.mov

BUG 10 ✅

Fixed, same design in login and password page.
Screenshot 2024-06-26 at 11 58 14

Thanks !

It is QA ✅ 💪

@florine2623 florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 26, 2024
@ps-jarvis
Copy link

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@boherm boherm added this to the 9.0.0 milestone Jun 26, 2024
@kpodemski
Copy link
Contributor

we need one approval @Progi1984 @nesrineabdmouleh

Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

Ok for UI Tests (tests/UI/tools/urls.ts).

Not in my scope for remaining files.

@kpodemski kpodemski merged commit 7ac829c into PrestaShop:develop Jun 26, 2024
46 checks passed
@kpodemski
Copy link
Contributor

thanks @jolelievre 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Type: Introduces a backwards-incompatible break develop Branch Feature Type: New Feature Needs autoupgrade PR QA ✔️ Status: check done, code approved
Projects
Status: To be tested
Development

Successfully merging this pull request may close these issues.

Clean legacy AdminLoginController and related references (links) and templates
7 participants