-
Notifications
You must be signed in to change notification settings - Fork 647
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: More helpful error state when trying to import from unprovisioned device #12397
Setup Wizard: More helpful error state when trying to import from unprovisioned device #12397
Conversation
Build Artifacts
|
Thanks @nucleogenesis - I confirm that now there's an error message if I attempt to import a learner from unprovisioned device. |
To @pcenov's note, we should probably add a prop to That would take care of filtering devices that would not be joinable or importable because they have no facility, which is a solid improvement. Although it doesn't go quite as far as the original issue's proposal of filtering those devices that actually have learners for import. I think the consensus was that we don't need to go that far. See my comment in regards to that |
if "timeout" not in kwargs: | ||
kwargs.update(timeout=self.timeout) | ||
|
||
timeout = kwargs.pop("timeout", self.timeout) |
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.
@bjester I saw the comment in the filters about wanting to pass the params to the API directly and started investigating. I was running into an error when I tried to pass the filter values as kwargs and discovered that the parent request method expects the params kwarg.
Looking at changing the front-end code to pass these values in to the API seemed like maybe out of scope for this PR, but does this look right and like it might help simplify the facility filters a bit. I was thinking I might just put that into the apiParams
object created in SelectDeviceForm but paused to get your thoughts 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.
Yeah so I think the changes that are needed are external to this client class. This class is a generic client utility. This means we rather need to update the APIs that are using it to pass the params.
@radinamatic @bjester I've updated this with changes that I think are in line with Blaine's suggestion which hides no-facility devices altogether when a particular prop is passed. I'm feeling a little unsure of my approach so I look forward to your thoughts on the code Blaine! |
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.
More comments. The primary blocking change is the updates to the network client. I'm quite sure those changes are likely breaking.
kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/SelectDeviceForm.vue
Outdated
Show resolved
Hide resolved
if "timeout" not in kwargs: | ||
kwargs.update(timeout=self.timeout) | ||
|
||
timeout = kwargs.pop("timeout", self.timeout) |
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.
Yeah so I think the changes that are needed are external to this client class. This class is a generic client utility. This means we rather need to update the APIs that are using it to pass the params.
50dca78
to
c75c06f
Compare
kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/SelectDeviceForm.vue
Outdated
Show resolved
Hide resolved
… user's selection between navigations)
… have no facilities by prop
{} => "are there any facilities at all?" { id: <val> } => "are there any facilities that match this id?" this ensures that we properly handle the case where there are no facilities
c75c06f
to
1b7a759
Compare
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 LGTM
); | ||
|
||
// If we're filtering a particular facility | ||
if (Object.keys(facilityFilter).length > 0 || props.filterByHasFacilities) { | ||
apiParams.subset_of_users_device = false; |
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.
It maybe unexpected that we filter out SoUDs when applying props.filterByHasFacilities
, but since it's a new prop, we don't have any use cases, so it's def fine until we do.
Hi @radinamatic - I confirm that the issue described in #12066 is fixed now and there are no new issues observed while regression testing. |
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.
Manual QA passes, good to go! 💯 👏🏽
Readthedocs build appears to have stalled, and has nothing to do with this PR. Merging! |
1353e99
into
learningequality:release-v0.17.x
Summary
2024-07-02.16-07-59.mp4
hideContinue
prop toOnboardingStepBase
to hide the Continue button there when we have this error stateReferences
Fixes #12066
Reviewer guidance
See the issue comment here for replication path -- note that I kinda had to go back and forth a few times through the flow - never actually importing anything - before I got the device to show up.
Overall, does the workflow feel okay?