Gives indication of user having permission to join the facility not showing the devices that don't have sign up enabled. #10703
Conversation
In this, I have added a basic logic of how the problem will be solved.
Build Artifacts
|
There was a problem hiding this comment.
I'll hold approval until @pcenov / @radinamatic has a chance to do some manual QA here to be sure everything works as expected.
Overall, the code looks good. I added one thought about seemingly duplicitous code in how you're fetching facilities -- but I may be misreading there (cc @rtibbles @akolson ?) - so let's see what the outcome of some discussion around this brings us in the mean time.
kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/SelectDeviceForm.vue
Outdated
Show resolved
Hide resolved
kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/SelectDeviceForm.vue
Outdated
Show resolved
Hide resolved
|
@Wck-iipi I'm not seeing why the tests are failing in the Github Actions output - can you run |
|
@nucleogenesis Sorry about the test cases failing, I ran tests on my branch and then on develop and both were failing day before yesterday so I thought my code didn't break anything. |
There was a problem hiding this comment.
@Wck-iipi I learned in a follow-up discussion with @bjester that the logic used here is duplicitous of code already available between useDevices and the SelectDeviceModalGroup/api.js.
This logic I believe should instead implement a filtering function which checks if a device has a facility with sign-ups allowed. This would be akin to the facilityIsAvailableAtDevice function.
However, the new function would just take the deviceId and return if any of the facilities have the ability to allow sign ups like this (I've not tested this):
export function deviceFacilityCanSignUp(deviceId) {
return NetworkLocationResource.fetchFacilities(deviceId)
.then(({ facilities }) => {
return facilities.some(f => f.learner_can_sign_up));
});
}
In this case then, we can get the list of devices using useDevicesForLearnOnlyDevice from useDevices to get a pre-filtered list of devices which is necessary for this particular case. Then iterate over these devices, calling the new function above for each device, and you can then know which devices should be shown in this case.
In this case now the SelectDeviceForm doesn't have to worry about maintaining a list of facilities, but rather can just hone in on which devices are useful in a particular case.
The change requests here are substantial -- essentially reworking most of what you've added. This issue is also a relatively urgent fix in our pipeline, which I want to share because we don't expect that your contributions will necessarily align with our plans.
If you'd like to take another shot this week, please let me know along with any questions and I'll do my best to clarify. If not, then I totally understand and greatly appreciate the work you've put in on this. It opened up a greater discussion around keeping our code extensible which is invaluable in any case.
|
@nucleogenesis I would love to continue to work on this. |
|
@nucleogenesis I have made some changes. Is this in the right direction? and would love your guidance. Thanks. |
|
I wonder if maybe In any case, perhaps if ... I think ... I hope that helps 😅 The latest commit looks like you're headed in the right direction 🙂 🚀 |
|
@nucleogenesis I have added the required code. However, this won't pass 2 tests. The first one sends above data and expects the KRadioButton to be enabled. But it is disabled as no information about the SignUp is provided(this line to be exact is causing the KRadioButton of dynamic device to be disabled). I can update the test case if you want to. About the second test case failing, I have absolutely no idea why it is failing, |
kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/SelectDeviceForm.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
I apologize for my delay in re-reviewing your work here I should have come back to this much sooner.
I have tested it and it works as expected and the code updates all look great to me.
I'm going to approve and merge this. From our whole team, thank you so much for taking this complex and critical issue and being patient with our feedback here as I know you have gone through your exams during this time. Your work is greatly appreciated and I hope we get to work with you again!
Please feel free to submit another PR adding your name and Github username to the AUTHORS.md file in the root of the repo if you'd like to be included there ❤️
| @@ -186,6 +190,24 @@ | |||
| const discoveredDevices = computed(() => get(devices).filter(d => d.dynamic)); | |||
| const savedDevices = computed(() => get(devices).filter(d => !d.dynamic)); | |||
|
|
|||
| const isLoading = ref(false); | |||
There was a problem hiding this comment.
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?
I'm not seeing anywhere that this ref's value is being read back. This could be used to conditionally put a <KCircularLoader /> component (which is globally available, no need to import it from anywhere) in place of where either the data or the "no data" message should go?
| @@ -356,6 +379,14 @@ | |||
| this.$emit('removed_address'); | |||
| }); | |||
| }, | |||
| canLearnerSignUp(id) { | |||
| if (this.LODDevicesWithSignUpFacility) { | |||
There was a problem hiding this comment.
Because this is an asyncComputed property I think you should be using isLoading here as well because if the async operation hasn't resolved then we can just return false.
This is a place where managing tests can get really tough because you'll need to be sure that the async data is available, so I suspect that in the failing test you'll need to be sure that this new method is represented properly.
|
Actually I'll fix the tests and push to your remote so we can get this merged 😄 |
Summary
This does it by copying the logic for getting facility's information from SelectFacility and shows the devices accordingly. If the sign up is allowed, only then device is shown.
References
Fixes #10313
Reviewer guidance
Please test whether devices are not shown when permission to sign up is not given.
Testing checklist
PR process
Reviewer checklist
yarnandpip)