Connection status for network locations with refactoring of network discovery hooks #10133

bjester · 2023-02-21T18:19:09Z

Summary

Overall this supports many new 0.16 features beyond the original scoped issue focused on the 'Select network device' modal, through providing access to connection metadata for network locations

Refactors zeroconf listeners to operate through tasks/jobs and to allow 'discovery' of static locations when there's a network change
Integrates new connection status metadata into network location models with appropriate coordination for keeping it up-to-date through new tasks/jobs
Adds feature for establishing network discovery hooks within individual Kolibri plugins
Refactors 'Select network device' modal to consolidate static/dynamic address handling in the frontend
~~Adds new strings for surfacing connection status to the user in the 'Select network device' modal~~
Fixes undocumented bug where device list wouldn't be filtered when intended to be filtered to those having a specific facility or channel
Re-enables frontend tests for 'Select network device' modal that were marked as skipped

References

Supports #10066
Addresses #9598 (potential followups below)
Resolves #7374

Followup tasks

Followup to integrate 'Connection testing' modal #10153
Other misc followup tasks #10186

Reviewer guidance

Open 'Select network device' modal for syncing multiple facilities, with network instances that have and haven't the same facility
Open 'Select network device' modal for importing channel content, with network instances that have and haven't the same channel
In any situation, connect and disconnect from networks while 'Select network device' modal is open
Use new 'Change facility' workflow when provisioned as on_my_own_setup
Add and remove manually added devices
Verify devices discovered through Zeroconf

How it works

When you open the 'Select network device' modal:

it should fetch all dynamically discovered devices on the network and any manually added (static) devices
it should filter and hide devices that do not have the same facility when the intent is to sync a particular facility
it should filter and hide devices that do not have the same channel when the intent is to import more of a particular channel
it should show all devices when the intent is to import a facility or channel not already on the device

While the 'Select network device' modal is open:

it should disable selection of any device if the device becomes unavailable (disconnects from the network, firewall issue, doesn't respond to requests, etc.)
it should periodically fetch any new dynamically discovered devices on the network, showing the loading spinner when doing so
it should periodically test the connection for each, showing the loading spinner when doing so
it should show the message Some devices are not responding. ... if any device becomes unavailable (disabled)

~~When the 'Select network device' modal is submitted:~~ needs #10153

Screenshots

Modal: at open	Modal: generic issue Modal: at submit	Modal: after submit failure

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

jredrejo

Thanks for this im pressive piece of code.
I'll be using some of these composables in my upcoming code .
I can follow most of the code and looks good to me (for some parts of the code I'm not familiar I'm not sure there will be other devs who can say more).

However, it is not perfect :) For the kolibri/plugins/user_profile/assets/src/views/ChangeFacility/SelectFacility.vue component, I've found some problems that I've commented inline.

kolibri/plugins/user_profile/assets/src/views/ChangeFacility/SelectFacility.vue

jredrejo · 2023-02-22T17:51:27Z

kolibri/core/assets/src/views/sync/SelectAddressModalGroup/SelectAddressForm.vue

+          } else {
+            this.isSubmitChecking = false;
+            switch (device.connection_status) {
+              case ConnectionStatus.Unknown:


As break does not exist, this will go to the default leg of the switch, not sure if this is done on purpose

It doesn't actually go to the default leg, it gets combined with the next switch for ConnectionStatus.ConnectionFailure

can device.connection_status be ConnectionStatus.Unknown & ConnectionStatus.ConnectionFailure at the same time? If not I only see the default leg as possible without the break. If it can, I don't see the need to query for ConnectionStatus.Unknown
In summary, I don't feel comfortable removing the break statement inside a switch, it removes the readability that switch adds over a series of chained if-else and there are usually cleaner options.

In this situation, it becomes an OR statement on either ConnectionStatus.Unknown or ConnectionStatus.ConnectionFailure:

> let x = 'a'; undefined > switch (x) { ... case 'a': ... case 'b': ... console.log('case a or b') ... break; ... default: ... console.log('no case') ... } case a or b

This type of situation is exactly why would tend to use a switch, because you can map multiple values to a single value with more clarity than a typical block of multiple if statements

bjester · 2023-02-23T15:57:46Z

kolibri/core/assets/src/api-resources/index.js

+  NetworkLocationResource,
+  StaticNetworkLocationResource,
+  DynamicNetworkLocationResource,
+} from './networkLocation';


Relocated so I could add methods specifically for these resources.

bjester · 2023-02-23T15:58:47Z

kolibri/core/assets/src/composables/useMinimumKolibriVersion.js

-      return false;
-    };
+    }
+    return false;
  });

  return {


Minor changes here to allow setting the version once at invocation of the composable.

bjester · 2023-02-23T15:59:22Z

kolibri/core/assets/src/mixins/commonSyncElements.js

-        base_url,
-        nickname: device_name,
-      }).save();
-    },


bjester · 2023-02-23T16:08:52Z

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

 }

-function versionCompare(version) {


Replaced with @jredrejo's useMinimumKolibriVersion

bjester · 2023-02-23T16:10:06Z

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

-
-    // If facilityId is not provided, then we are at the initial Facility Import workflow
-    // disable any locations unless it is unavailable/offline
-    return locations.map(location => ({ ...location, hasContent: location.available }));


Consolidated this logic out of the API utilities, similar for both channels and facilities, into filtering composable structure

bjester · 2023-02-23T16:18:20Z

kolibri/core/assets/src/views/sync/SelectAddressModalGroup/useConnectionChecker.js

+ */
+export default function useConnectionChecker(
+  devices,
+  { threshold = 5, interval = 2, concurrency = 3 } = {}


This updates the connection status when keeping it refreshed is important, like the user sitting with the network device modal open.

With these parameters, I tried to balance the situation where there might be many devices on the network. Open to tweaking these

bjester · 2023-02-23T16:22:54Z

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

+        .filter(d => get(unavailableIds).indexOf(d.id) < 0)
+        .map(d => {
+          // set unavailable if we haven't determined if it should be filtered out yet
+          d.available = get(availableIds).indexOf(d.id) >= 0;


This was a fly-by decision to maintain visibility into the underlying processing, by showing locations as unavailable until we've determined the filtering result from getIsAvailable. If it takes a long time to call getIsAvailable, without this the user would see an empty list. The downside is that if the device gets filtered out, the device would vanish from the list.

bjester · 2023-02-23T16:24:10Z

kolibri/core/auth/management/utils.py

    controller = MorangoProfileController(PROFILE_FACILITY_DATA)
-    network_connection = controller.create_network_connection(get_baseurl(baseurl))
+    network_connection = controller.create_network_connection(get_baseurl(address))


Overall, using address here until it's been resolved to a base_url

bjester · 2023-02-23T16:26:40Z

kolibri/core/content/tasks.py

            ):
                raise IncompatibleVersionError(
                    "Remote Kolibri instance must be 0.16.0 or higher"
                )
-            peer["base_url"] = client.baseurl


This appeared to be a bug. baseurl doesn't exist on NetworkClient

bjester · 2023-02-23T16:27:08Z

kolibri/core/device/api.py

-        except Exception as e:
-            raise ValidationError(detail=str(e))
-
-


Consolidated with existing endpoint in discovery/api.py

bjester · 2023-02-23T16:49:08Z

kolibri/core/discovery/test/test_network_utils.py

+                "https://192.168.0.1/",
+            ],
+        )
+


If the passed in URL already had a port, it would produce two URLs with the same port if the port was one of the standard ports. This test ensures we dedupe the url variations.

bjester · 2023-02-23T16:50:34Z

kolibri/core/discovery/utils/network/client.py

+        self.remote_ip = None
+
+    @classmethod
+    def build_for_address(cls, address, timeout=None):


Originally, this logic always occurred in the constructor of the NetworkClient. By moving it to a factory method, it makes the client a little more useful as a utility

bjester · 2023-02-23T16:51:12Z

kolibri/core/discovery/utils/network/client.py

+                timeout = (DEFAULT_CONNECT_TIMEOUT, DEFAULT_ASYNC_READ_TIMEOUT)
+            else:
+                # if we're within a request thread, then we limit it for an overall time
+                timeout = (DEFAULT_CONNECT_TIMEOUT, DEFAULT_SYNC_READ_TIMEOUT)


I tried to make the client a bit smart on handling timeouts when it's going to try many URL variations

bjester · 2023-02-23T16:53:31Z

packages/kolibri-tools/lib/apiSpecExportTools.js

@@ -103,6 +103,7 @@ const distSpecFilePath = path.resolve(__dirname, '../dist/apiSpec.json');
 try {
  apiSpec = specModule(specFilePath);
 } catch (e) {
+  console.error(e);


Logging this error helped clarify what was going wrong when confronted with an error about missing apiSpec.json

bjester · 2023-03-09T22:22:52Z

kolibri/core/assets/src/composables/useMinimumKolibriVersion.js

-      return false;
-    };
+    }
+    return false;


Refactored this for re-use with new device composables. Its change in interface didn't affect existing usage.

github-actions · 2023-03-10T20:09:12Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a8.dev0_git.190.g0dcc9d53.pex
Windows Installer (EXE)	kolibri-0.16.0a8.dev0+git.190.g0dcc9d53-unsigned.exe
Debian Package	kolibri_0.16.0a8.dev0+git.190.g0dcc9d53-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a8.dev0+git.190.g0dcc9d53-0.3.0.dmg
Source Tarball	kolibri-0.16.0a8.dev0+git.190.g0dcc9d53.tar.gz
WHL file	kolibri-0.16.0a8.dev0+git.190.g0dcc9d53-py2.py3-none-any.whl

jredrejo

I have tried to test this after the latest changes and there's no way, either with a previous db or starting it from scratch. I always get this error:

ERROR    2023-03-13 20:22:49,780 Error in 'RUN' listener <bound method ZeroConfPlugin.RUN of <kolibri.utils.server.ZeroConfPlugin object at 0x7f719deedc00>>
Traceback (most recent call last):
  File "/datos/le/mio/kolibri/venv/lib/python3.10/site-packages/magicbus/base.py", line 273, in publish
    result = listener(*args, **kwargs)
  File "/datos/le/mio/kolibri/kolibri/utils/server.py", line 302, in RUN
    from kolibri.core.discovery.utils.network.search import NetworkLocationListener
  File "/datos/le/mio/kolibri/kolibri/core/discovery/utils/network/search.py", line 3, in <module>
    from kolibri.core.discovery.tasks import add_dynamic_network_location
  File "/datos/le/mio/kolibri/kolibri/core/discovery/tasks.py", line 222, in <module>
    def add_dynamic_network_location(broadcast_id, instance):
  File "/datos/le/mio/kolibri/kolibri/core/tasks/decorators.py", line 41, in register_task
    return RegisteredTask(
  File "/datos/le/mio/kolibri/kolibri/core/tasks/registry.py", line 218, in __init__
    raise ValueError(
ValueError: High priority tasks must specify a status_fn to inform the user of why it is important

Note: I've made a make clean and pip/yarn install and rebuilt everything to avoid conflict with previous code without success

bjester · 2023-03-13T19:27:11Z

@jredrejo yeah, I need to push the fix for that. I had talked with @rtibbles about this new status function constraint for high priority tasks-- they need wired up to the status functions to pass the validation.

jredrejo · 2023-03-13T19:46:51Z

@jredrejo yeah, I need to push the fix for that. I had talked with @rtibbles about this new status function constraint for high priority tasks-- they need wired up to the status functions to pass the validation.

ok, please, let me know when I can re-test again. I'm very interested in using the composables you wrote here

- Refactors zeroconf listeners to operate through tasks/jobs - Integrates new connection status metadata into network device management - Adds feature for network discovery hooks within Kolibri plugins - Allows support for triggering connection checking for static locations - Refactors frontend to consolidate static/dynamic address handling - Adds new strings for surfacing connection status to the user

…n checking logic

jredrejo

Tested and works great in the frontend. I'm looking forward to using it soon.
Thanks!

rtibbles

Code makes sense on a read through, and all previous comments have been addressed.

If there are any issues here, we're going to find them by using this!

bjester added the TAG: user strings Application text that gets translated label Feb 21, 2023

bjester added this to the Kolibri 0.16 Release: Existing Projects/General/Maintenance milestone Feb 21, 2023

bjester added the changelog Important user-facing changes label Feb 22, 2023

jredrejo requested changes Feb 22, 2023

View reviewed changes

bjester changed the title ~~WIP Refine network device selection~~ WIP Connection status for network locations with refactoring of network discovery triggers Feb 22, 2023

bjester changed the title ~~WIP Connection status for network locations with refactoring of network discovery triggers~~ WIP Connection status for network locations with refactoring of network discovery hooks Feb 22, 2023

bjester commented Feb 23, 2023

View reviewed changes

kolibri/core/assets/src/mixins/commonSyncElements.js

base_url,

nickname: device_name,

}).save();

},

Copy link

Member Author

bjester Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused

bjester commented Feb 23, 2023

View reviewed changes

bjester changed the title ~~WIP Connection status for network locations with refactoring of network discovery hooks~~ Connection status for network locations with refactoring of network discovery hooks Feb 23, 2023

bjester assigned radinamatic Feb 23, 2023

bjester requested review from rtibbles and jredrejo February 23, 2023 19:51

This was referenced Feb 28, 2023

Integrate post-selection connection testing modal #10153

Open

Finalize integration device connection testing #10186

Open

bjester changed the title ~~Connection status for network locations with refactoring of network discovery hooks~~ [REBASING] Connection status for network locations with refactoring of network discovery hooks Mar 9, 2023

bjester mentioned this pull request Mar 9, 2023

Add is_local attribute to NetworkLocation #10220

Closed

bjester commented Mar 9, 2023

View reviewed changes

bjester force-pushed the addressing-network branch from 513c95d to c885a53 Compare March 10, 2023 19:52

jredrejo requested changes Mar 13, 2023

View reviewed changes

bjester mentioned this pull request Mar 14, 2023

Strings==: for when multiple devices are not responding #10234

Merged

9 tasks

bjester added 13 commits March 14, 2023 10:04

Fix content tests mocking of NetworkClient

49d4107

Reset faults and clear jobs; fix tests in py2.7

8ed73ea

Only cancel discovery jobs if they exist

b82105f

Function wrapping fixes for py2.7

4c0396e

Consolidate 'remote facilities' API and fix broken change facility view

7c057f8

Fix mistake with broken facility deduplication

d65d553

Code review: priority handling of connection testing

ffdf43a

Capture 'NotFound' as same as connection failure

5ce5ae9

Ensure connection reset is called on network change; narrow connectio…

b1d5abf

…n checking logic

Remove broken testing spec that doesn't really add coverage

c2dbc30

Cleanup strings

f00afbe

Implement discovery task status functions

10202d5

bjester force-pushed the addressing-network branch from c885a53 to 10202d5 Compare March 14, 2023 17:15

bjester changed the title ~~[REBASING] Connection status for network locations with refactoring of network discovery hooks~~ Connection status for network locations with refactoring of network discovery hooks Mar 14, 2023

bjester requested a review from jredrejo March 14, 2023 17:22

jredrejo approved these changes Mar 14, 2023

View reviewed changes

rtibbles approved these changes Mar 14, 2023

View reviewed changes

rtibbles merged commit 1f9e70f into learningequality:develop Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection status for network locations with refactoring of network discovery hooks #10133

Connection status for network locations with refactoring of network discovery hooks #10133

bjester commented Feb 21, 2023 •

edited

Loading

jredrejo left a comment

jredrejo Feb 22, 2023

bjester Feb 22, 2023

jredrejo Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Feb 23, 2023

bjester Mar 9, 2023

github-actions bot commented Mar 10, 2023 •

edited

Loading

jredrejo left a comment

bjester commented Mar 13, 2023

jredrejo commented Mar 13, 2023

jredrejo left a comment

rtibbles left a comment

Connection status for network locations with refactoring of network discovery hooks #10133

Connection status for network locations with refactoring of network discovery hooks #10133

Conversation

bjester commented Feb 21, 2023 • edited Loading

Summary

References

Followup tasks

Reviewer guidance

How it works

Screenshots

Testing checklist

PR process

Reviewer checklist

jredrejo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2023 • edited Loading

jredrejo left a comment

Choose a reason for hiding this comment

bjester commented Mar 13, 2023

jredrejo commented Mar 13, 2023

jredrejo left a comment

Choose a reason for hiding this comment

rtibbles left a comment

Choose a reason for hiding this comment

bjester commented Feb 21, 2023 •

edited

Loading

github-actions bot commented Mar 10, 2023 •

edited

Loading