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

Setup Wizard > LOD > Create a new user - No indication that the user doesn't have permission to join the facility #10313

Closed
pcenov opened this issue Mar 24, 2023 · 8 comments · Fixed by #10703
Assignees
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) bug Behavior is wrong or broken P0 - critical Priority: Release blocker or regression

Comments

@pcenov
Copy link
Member

pcenov commented Mar 24, 2023

Observed behavior

If on my facility the option 'Allow learners to create accounts' is disabled, when a user attempts to join the facility through the 'LOD > Create a new user account for an existing facility' workflow then the user is not being notified that they don't have permission to join the facility.

Expected behavior

The user should not be allowed to select that facility in the first place. As specified in Figma, in that case the facility should be grayed out and stating: You don't have permission to join this facility

Steps to reproduce the issue

  1. Install the the latest develop build.
  2. Setup a full facility and disable 'Allow learners to create accounts' at Facility > Settings
  3. On another device attempt to create a Learn-only device by going through the setup wizard and selecting the 'Create a new user account for an existing facility' option

Video

2023-03-24_16-07-53.mp4

Console error

{"status":403,"data":"[{\"id\":\"PERMISSION_DENIED\",\"metadata\":{\"view\":\"Public Sign Up List\"}}]"}

Usage Details

Windows 10, Ubuntu 22 - Chrome, Firefox

@pcenov
Copy link
Member Author

pcenov commented Mar 24, 2023

@radinamatic

@radinamatic radinamatic added bug Behavior is wrong or broken P0 - critical Priority: Release blocker or regression APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) labels Mar 24, 2023
@Wck-iipi
Copy link
Contributor

@pcenov @radinamatic I would like to work on this issue. But first I want to ask whether you have setup these 2 devices using virtualbox and if there is another way to setup two devices. Thanks.

@nucleogenesis
Copy link
Member

@Wck-iipi thanks for your interest in working on this

Getting setup to test this should be easiest if you use the latest PEX from our latest alpha release - you can run this with python path/to/the/kolibri.pex. I suggest when you run this, you prepend KOLIBRI_HOME=~/.kolibri-pex (or whatever name you want in place of .kolibri-pex -- this will make sure that the PEX is run on its own fresh home folder. Go through the onboarding flow "Full Facility" and create a new facility. Be sure to then set the setting as shown in @pcenov 's example above.

Then you can simultaneously run the kolibri dev server - again I suggest using a fresh KOLIBRI_HOME ie, KOLIBRI_HOME=~/.kolibri-dev -- you can then run your devserver and have everything in place to test fixes. When you get into the device selection, you can then put the localhost:8080 URL to the PEX-run server, which should then select your facility (or, alternatively let you know there are no facilities available on that device).


One bit of guidance I have is that I suggest you look at the kolibri/plugins/user_auth/.../SignUpPage.vue file to see how a similar situation is handled there. In a Full Facility, learners may be able to join directly to the facility. This differs from the issue at hand here in that this particular case is during the Onboarding where one joins a Facility as a sort of "remote user". This issue and the Sign Up flow should be using the same underlying components to identify which facilities permit users to join them.


For this to be resolved, we'll need to see:

  • The user will either not see facilities with the setting set to disallow sign-ups, or such facilities will be "disabled" themselves as choices (ie, the user can see them listed, but cannot select them).
  • The user should receive a message that no facilities are available to join if all facilities on a device are set to disallow sign-ups

As a stretch goal, I'd say we should try to:

  • If possible, we can borrow messaging from the SignUpPage flow to indicate to the user that they cannot join a particular facility -- note that we cannot add new messaging at this time, so don't I'd leave this particular need to our team to handle internally.

I know that is a lot :-D so please let me know if you're interested in taking this on, all things considered. Happy to answer any additional questions as well.

@Wck-iipi
Copy link
Contributor

@nucleogenesis Thank you so much for such a detailed response. I can now finally replicate this error with PEX file. I am interested in taking this on and would love to work on this issue.

@nucleogenesis
Copy link
Member

@Wck-iipi thanks! Tag me if you run into any issues! We do hope for this to be resolved within the next few weeks so any help will be greatly appreciated.

@nucleogenesis
Copy link
Member

@Wck-iipi -- do you still plan to work on this?

If not I will take this on myself.

@Wck-iipi
Copy link
Contributor

@nucleogenesis Sorry for inactivity. My semester exams just finished. Yes I am still interested and will issue pull request within next 2 days.

@Wck-iipi
Copy link
Contributor

@nucleogenesis I have added the pull request. I have tried to change as little things as I can in this PR so that it is easy to check. I copied the intial logic from SelectFacility, as I couldn't find any other way to check the learner_can_sign_up field. I, however, still have some questions I want to ask in it so that I can change it:

  1. In the original PR, a lot of fields like base_url, name, etc. are not required. Should I remove them to get code as small as possible?
  2. Instead of discoveredDevices and savedDevices, should I just have discoveredSignUpDevices and savedSignUpDevices which will only have those devices which have sign up instead of having another field availableFacilites and comparing through all of its elements?
  3. Is it okay to show "There are no devices yet" when there are devices but there are none that can sign up or should I use another string?
    If there is something I have done wrong or isn't upto standard please tell me. Again, I apologise for the late PR as I was busy with college work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) bug Behavior is wrong or broken P0 - critical Priority: Release blocker or regression
Projects
None yet
4 participants