Instead of returning an access denied page on /commerce_pos/login for logged in users, either a message explaining the user is already logged in should be displayed or it should redirect to the register selection page. Having an access denied page might be confusing for users.

CommentFileSizeAuthor
#24 nicer_login-2986069-23.patch2.8 KBsmccabe
#21 nicer_login-2986069-21.patch2.26 KBgmem
#20 nicer_login-2986069-20.patch15.96 KBgmem
#17 nicer_login-2986069-7.patch2.3 KBgmem
#16 nicer_login-2986069-6.patch2.3 KBgmem
#10 nicer_login-2986069-5.patch2.51 KBgmem
#8 nicer_login-2986069-4.patch2.47 KBgmem
#6 nicer_login-2986069-3.patch2.44 KBgmem
#5 nicer_login-2986069-2.patch2.66 KBgmem
#2 nicer_login-2986069-1.patch1.23 KBgmem

Comments

gmem created an issue. See original summary.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-1.patch1.23 KB

Small patch that checks if a user is logged in, then redirects them to the main POS interface.

gmem’s picture

gmem commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: nicer_login-2986069-1.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-2.patch2.66 KB

Wrote a small test to verify redirect works, unsure why other test is failing but added new permission to test user, which should hopefully fix this issue.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-3.patch2.44 KB

Changed out the requirement for /commerce_pos/login to _access: 'TRUE', which seems to have fixed the tests.

gmem’s picture

gmem commented
Status: Needs work » Needs review
gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-4.patch2.47 KB

Needs to get route from commerce_pos.main route instead of hardcoding the route, otherwise testbot fails. In practice this would work, but only if the site is not installed to a subdirectory.

Status: Needs review » Needs work

The last submitted patch, 8: nicer_login-2986069-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gmem’s picture

gmem commented
Status: Needs work » Needs review
StatusFileSize
new nicer_login-2986069-5.patch2.51 KB

Turns out the redirect method wanted a string, not a Url object. Fixed.

Status: Needs review » Needs work

The last submitted patch, 10: nicer_login-2986069-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gmem’s picture

gmem commented
Status: Needs work » Needs review

Failed test doesn't appear to be related to the patch, although worth an investigation.

smccabe’s picture

smccabe commented

I also was wondering if maybe we should just log the user out and use this as a sort of "switch user" screen as well, since POS is often a shared computer type situation.

Status: Needs review » Needs work

The last submitted patch, 10: nicer_login-2986069-5.patch, failed testing. View results

smccabe’s picture

smccabe commented

kk confirmed with Ted, going to the login page should just log you out and show the login page. We could put a message indicating that you've been logged out.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-6.patch2.3 KB

Rerolled and changed redirect to a logout and Drupal message, would it be better to implement the message into the Twig template itself or just leave it as a Drupal message?

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-7.patch2.3 KB

Tweaked the test so it tests for the message instead of the redirect previously

gmem’s picture

gmem commented
Status: Needs work » Needs review
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work 
+++ b/src/Controller/PosCashierLoginPage.php
@@ -16,6 +16,11 @@ class PosCashierLoginPage extends ControllerBase {
+    if (\Drupal::currentUser()->isAuthenticated()) {

current_user and MessengerTrait are already available in the parent class. You don't need to load them again from services.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-20.patch15.96 KB

Removed the loading of those classes, also put message between double quotes to avoid escaping '.

gmem’s picture

gmem commented
StatusFileSize
new nicer_login-2986069-21.patch2.26 KB

Wrong patch file, updated.

gmem’s picture

gmem commented
Status: Needs work » Needs review
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Reviewed & tested by the community
smccabe’s picture

smccabe commented
StatusFileSize
new nicer_login-2986069-23.patch2.8 KB

Slightly updated the test to check that we can actually login and not just that the message shows, otherwise good and I'll merge it in.

  • smccabe committed bcf9028 on 8.x-2.x authored by gmem 
    Issue #2986069 by gmem, smccabe, subhojit777: Nicer message / redirect...
smccabe’s picture

smccabe commented
Status: Reviewed & tested by the community » Fixed

Fixed, thanks guys!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.