-
Notifications
You must be signed in to change notification settings - Fork 694
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
Do a correct check to know if the facility has been imported #12077
Do a correct check to know if the facility has been imported #12077
Conversation
Build Artifacts
|
Hi @jredrejo - I am not able to replicate the expected behavior of seeing the 'Select a source' modal. Here's a video of what I am doing and observing. Let me know if I should try anything else: 2024-04-11_14-30-09.mp4 |
@pcenov I have no clue of what might be happening there 🤷♂️ |
Hi @jredrejo, here's what's in the 'Session storage': |
@pcenov |
Hi @jredrejo - I've tried importing different facility, tried using a different browser and so on but I'm still nor able to see the 'Select a source' modal. If you can, please share a video of what you are doing on your end. Thanks! |
@pcenov sure, let's check if we can spot any difference: Peek.2024-04-12.17-43.mp4 |
Hi @jredrejo - after additional testing I think I've found the reason for not seeing the modal - the facility which I was importing was created through the 'On my own' path. If I create a full facility and import it - then I am able to see the 'Select a source' modal. Importing a full facility: full.facility.import.mp4Importing an 'On my own' facility: 2024-04-15_10-35-27.mp4 |
Also the 'Add new device' link is not working (tested both in Chrome and Firefox): 2024-04-15_12-20-21.mp4 |
@pcenov please create a follow up issue on this. Let's not merge different problems in the same issue
hi @pcenov I see now clearly the problem and I can reproduce it perfectly
After a decission is made I'll change the code accordingly. |
… the setup wizard
@@ -19,6 +19,8 @@ | |||
/> | |||
<SelectDeviceModalGroup | |||
v-if="showSelectAddressModal" | |||
:filterByFacilityCanSignUp="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to filter by this? We should be able to import facilities even if they don't allow sign up, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, it was a unneeded code I left when testing it, but deleted it in another commit a few seconds after this one. If you look at the complete PR code this line is not there 😉
@pcenov it would be great if you can retest it. Facilities with an "on your own setup" should not be importable now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes make sense to me. @pcenov if you can do a final pass here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jredrejo - I confirm that now facilities created through 'On my own' can't be imported in the Setup Wizard.
Summary
Using the FacilityViewset api to check if a facility has been imported after the setup wizard finishes does not work because in https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/api.py#L617-L629 it checks PUSH transfers, that don't happen when importing a facility.
This PR changes the setup wizard behavior to solve it.
It also fixes an error observed while testing the wizard due to a incorrect ref name.
References
Closes: #10959
Closes: #11769
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)