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

List Users Login Flow in App Context #6962

Conversation

nucleogenesis
Copy link
Member

@nucleogenesis nucleogenesis commented May 29, 2020

Summary

Revises the Sign In flow per designs linked below in References.

  • When in app context and <= 16 users in Facility, we list the users by name which simulates typing a username.
  • If a password is NOT required - clicking a user in a list (or entering your username) will log you in.
  • If a password IS REQUIRED - clicking a user in a list (or entering your username) will present you with a password text field, which will log you in or show errors.
  • If a user does not have a password - but the setting is changed to requiring a password, the user is asked to create a password. The user is logged in - their password is updated - then they're redirected to wherever they were going to in the first place.

Reviewer guidance

Setup

  • Have a device with <= 16 total users in a facility and select that facility at the sign-in page.
  • One or more of those users must be a Learner.
  • Have your handy SQLite browser app and open your database in it. In this browser there should be a "Browse Data" tab. Then a drop down menu below listing the table in the database. Go to the FacilityUser table there.

When you need a Learner to "have no password" - then you will need to edit the password column for that Learner in the DB Browser. Single-click to highlight then type to edit the cell directly to the value NOT_SPECIFIED and click "Write Changes" at the top. Now that Learner "has no password".

Things to Test Once

  • Turn off "Learners can see content without logging in" (aka turn off guest access). Then go to a URL pointing to a specific piece of content - which should take you to the Sign In page. Log in and make sure you are still redirected to the content.
  • Check that you can complete the Learner who has no password scenario even when the Facility Setting "Learners can change their own password" is turned off.

Things to Test In Each Scenario

  • Login as an Admin.
  • Login as a Learner who has no password. Create a password & sign in. Logout. Log back in with the password you made.
  • Login as a Learner who has a password.
  • Change a Learner who has no password to a Coach or Admin & login as that user (create a password and finish the form).
  • Change an Admin or Coach to a Learner and login as that Learner.

Scenarios

You have <= 16 users of all kinds in your facility and are NOT in app context.

In addition to the above "every scenario" items:

  • You should be presented first with a Username text field. Entering a username and clicking next will take you to password entry.
  • Test invalid usernames and passwords.
  • Test going back and forth between password and username fields.

You have <= 16 users of all kinds in your facility and ARE in app context.

In addition to the above "every scenario" items:

  • You should see the users listed as buttons.
  • Click a user should work the same as the previous scenario as if you entered a username and act the same from there.
  • Test going back and forth between password and users list fields

You have > 16 users of all kinds in your facility and ARE in app context.

  • You should see the same username text field flow as the first scenario.

References

Figma

Notion


Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@nucleogenesis nucleogenesis added the work-in-progress Not ready for review label May 29, 2020
@nucleogenesis nucleogenesis added this to the android-mvp milestone May 29, 2020
@nucleogenesis nucleogenesis added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Jun 4, 2020
@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #6962 into app-support will decrease coverage by 19.59%.
The diff coverage is 10.81%.

Impacted Files Coverage Δ
...ibri/core/assets/src/state/modules/core/actions.js 23.12% <0.00%> (ø)
.../assets/src/views/userAccounts/PasswordTextbox.vue 61.76% <0.00%> (ø)
kolibri/plugins/user/kolibri_plugin.py 83.33% <0.00%> (-7.58%) ⬇️
...plugins/user/assets/src/views/SignInPage/index.vue 29.10% <9.09%> (ø)
kolibri/core/auth/api.py 87.15% <62.50%> (-0.62%) ⬇️
.../plugins/learn/assets/src/modules/classes/index.js 0.00% <0.00%> (ø)
kolibri/core/assets/src/views/AppBar.vue 77.77% <0.00%> (ø)
...ets/src/views/components/KDropdownMenu/Example.vue 0.00% <0.00%> (ø)
...s/coach/assets/src/modules/questionDetail/index.js 12.50% <0.00%> (ø)
...evice/assets/src/views/ManageContentPage/index.vue 0.00% <0.00%> (ø)
... and 624 more

@nucleogenesis nucleogenesis marked this pull request as ready for review June 5, 2020 22:33
@indirectlylit
Copy link
Contributor

attempted to merge in latest app-support branch, resolving conflict in sign-in page related to remote-access setting from @jredrejo

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

This is looking good. I haven't fully finished testing all the scenarios but I'm going to merge in and file follow-up issues to expedite strings getting to develop.

A few initial things I'm seeing:

View content as guest isn't working

I get a 401 when accessing content as guest. Also the error reporting isn't working, which is probably unrelated:

2020-06-05 20 11 12

Styling

Learner usernames should not be capitalized:

image

Possible string updates

The more I look at these strings, the more awkward they seem to me. Proposed:

  • "Go back" -> "Switch user"
  • "Hi, " -> "Signing in as '_':

image

@indirectlylit indirectlylit merged commit 13b4f65 into learningequality:app-support Jun 6, 2020
@indirectlylit indirectlylit modified the milestones: android-mvp, 0.14.0 Jul 6, 2020
@jonboiser jonboiser added changelog Important user-facing changes and removed TODO: needs review Waiting for review labels Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important user-facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants