Fix setup wizard selection of network devices for sign up or import #11510

bjester · 2023-11-08T20:39:16Z

Summary

Reverts prior change that implemented filtering on whether a learner can signup
Implements, through refactoring, method for flexible filtering of facilities which is now supported through the selection form

With learner sign up disabled

Join	Import

With learner signup enabled

Join	Import

References

Fixes: #11496

Reviewer guidance

Tested the signup flow and channel import

Testing checklist

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

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

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

github-actions · 2023-11-08T21:20:47Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b8.dev0_git.59.gb2dd9923.pex
Windows Installer (EXE)	kolibri-0.16.0b8.dev0+git.59.gb2dd9923-unsigned.exe
Debian Package	kolibri_0.16.0b8.dev0+git.59.gb2dd9923-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b8.dev0+git.59.gb2dd9923-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b8.dev0+git.59.gb2dd9923-0.1.0-debug.apk
TAR file	kolibri-0.16.0b8.dev0+git.59.gb2dd9923.tar.gz
WHL file	kolibri-0.16.0b8.dev0+git.59.gb2dd9923-py2.py3-none-any.whl

nucleogenesis

The only blocking change request is the use of the conditional chaining operator.

Overall, this is a massive improvement. Every place there is a change, the result is more readable and easier to follow/understand. I really love this overall and I think that useDevices and the filtering business would be a great thing to present & discuss with the team if you'd be up for it.

kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/api.js

kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/useDevices.js

nucleogenesis · 2023-11-17T23:55:09Z

kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/useDevices.js

+    return () => Promise.resolve(true);
+  }
+
+  return useAsyncDeviceFilter(function deviceFacilityFilter(device) {


Does the parameter passed to useAsyncDeviceFilter have to be named or could it be an anonymous function?

Mostly asking because when I see something named, especially as specifically as deviceFacilityFilter I kind of expect it to be something that I need to know about elsewhere or later but it seems like this could just as well be called filterFn or nothing at all -- although I could totally be missing something elsewhere?

It doesn't have to be named. Although, there is a benefit when reading error stack traces, because anonymous functions would of course be anonymous and indistinguishable from other anonymous.

nucleogenesis · 2023-11-17T23:56:02Z

kolibri/core/assets/src/views/sync/SelectDeviceModalGroup/useDevices.js

+ * Produces a function that resolves with a boolean if Kolibri version is at least the specified
+ * @param {number} major
+ * @param {number} minor
+ * @param {number} patch
+ * @return {function(NetworkLocation): Promise<boolean>}


This is a beautiful improvement <3

I wish it showed the full diff so it was obvious here what it looked like before lol this is really so much nicer a way to do what we need to do here.

Yeah by making the filters (filter functions) more like object entities, it disconnects the 'inheritance' structure that was in use before and makes it more flexible

…Solution" This reverts commit 9025057, reversing changes made to 0e2dc3a.

nucleogenesis

Requested changes have been made. Thanks for this Blaine!

akolson

Changes look correct to me and code is understandable. Thanks @bjester

github-actions bot added APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) SIZE: medium labels Nov 8, 2023

github-actions bot added the APP: Learn Re: Learn App (content, quizzes, lessons, etc.) label Nov 8, 2023

bjester requested a review from nucleogenesis November 17, 2023 17:59

bjester force-pushed the network-selection branch from 037aabf to 2538d04 Compare November 17, 2023 19:11

bjester marked this pull request as ready for review November 17, 2023 19:11

bjester added the TODO: needs review Waiting for review label Nov 17, 2023

bjester added this to the Kolibri 0.16 Release: Bugs and Regressions milestone Nov 17, 2023

bjester requested review from marcellamaki and akolson November 17, 2023 19:12

nucleogenesis requested changes Nov 17, 2023

View reviewed changes

bjester added 6 commits November 21, 2023 13:58

Revert "Merge pull request learningequality#10703 from Wck-iipi/10313…

812e09e

…Solution" This reverts commit 9025057, reversing changes made to 0e2dc3a.

Refactor device filtering for optimal flexibility

5dcdc33

Update tests

1b61277

Fix bug that set unavailable instances to available

a4b97f7

Fix jest tests

edcc3eb

Use lodash get for optional chaining operator

addd5fe

bjester force-pushed the network-selection branch from 2538d04 to addd5fe Compare November 21, 2023 21:58

bjester requested a review from nucleogenesis November 21, 2023 22:03

nucleogenesis approved these changes Nov 22, 2023

View reviewed changes

akolson approved these changes Nov 27, 2023

View reviewed changes

bjester merged commit b88a804 into learningequality:release-v0.16.x Nov 27, 2023
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setup wizard selection of network devices for sign up or import #11510

Fix setup wizard selection of network devices for sign up or import #11510

bjester commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Nov 17, 2023

nucleogenesis Nov 18, 2023

bjester Nov 20, 2023 •

edited

Loading

nucleogenesis Nov 17, 2023

nucleogenesis Nov 18, 2023

bjester Nov 20, 2023

nucleogenesis left a comment

akolson left a comment

Fix setup wizard selection of network devices for sign up or import #11510

Fix setup wizard selection of network devices for sign up or import #11510

Conversation

bjester commented Nov 8, 2023 • edited Loading

Summary

With learner sign up disabled

With learner signup enabled

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Nov 8, 2023 • edited Loading

Build Artifacts

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Nov 17, 2023

Choose a reason for hiding this comment

nucleogenesis Nov 18, 2023

Choose a reason for hiding this comment

bjester Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

nucleogenesis Nov 17, 2023

Choose a reason for hiding this comment

nucleogenesis Nov 18, 2023

Choose a reason for hiding this comment

bjester Nov 20, 2023

Choose a reason for hiding this comment

nucleogenesis left a comment

Choose a reason for hiding this comment

akolson left a comment

Choose a reason for hiding this comment

bjester commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023 •

edited

Loading

bjester Nov 20, 2023 •

edited

Loading