-
Notifications
You must be signed in to change notification settings - Fork 648
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
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
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.
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
Outdated
Show resolved
Hide resolved
kolibri/plugins/user_profile/assets/src/views/ChangeFacility/SelectFacility.vue
Outdated
Show resolved
Hide resolved
} else { | ||
this.isSubmitChecking = false; | ||
switch (device.connection_status) { | ||
case ConnectionStatus.Unknown: |
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.
As break
does not exist, this will go to the default
leg of the switch, not sure if this is done on purpose
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 doesn't actually go to the default
leg, it gets combined with the next switch for ConnectionStatus.ConnectionFailure
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.
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.
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.
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
NetworkLocationResource, | ||
StaticNetworkLocationResource, | ||
DynamicNetworkLocationResource, | ||
} from './networkLocation'; |
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.
Relocated so I could add methods specifically for these resources.
return false; | ||
}; | ||
} | ||
return false; | ||
}); | ||
|
||
return { |
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.
Minor changes here to allow setting the version once at invocation of the composable.
base_url, | ||
nickname: device_name, | ||
}).save(); | ||
}, |
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.
Unused
} | ||
|
||
function versionCompare(version) { |
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.
Replaced with @jredrejo's useMinimumKolibriVersion
|
||
// 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 })); |
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.
Consolidated this logic out of the API utilities, similar for both channels and facilities, into filtering composable structure
*/ | ||
export default function useConnectionChecker( | ||
devices, | ||
{ threshold = 5, interval = 2, concurrency = 3 } = {} |
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.
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
.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; |
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.
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.
controller = MorangoProfileController(PROFILE_FACILITY_DATA) | ||
network_connection = controller.create_network_connection(get_baseurl(baseurl)) | ||
network_connection = controller.create_network_connection(get_baseurl(address)) |
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.
Overall, using address
here until it's been resolved to a base_url
): | ||
raise IncompatibleVersionError( | ||
"Remote Kolibri instance must be 0.16.0 or higher" | ||
) | ||
peer["base_url"] = client.baseurl |
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.
This appeared to be a bug. baseurl
doesn't exist on NetworkClient
except Exception as e: | ||
raise ValidationError(detail=str(e)) | ||
|
||
|
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.
Consolidated with existing endpoint in discovery/api.py
"https://192.168.0.1/", | ||
], | ||
) | ||
|
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.
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.
self.remote_ip = None | ||
|
||
@classmethod | ||
def build_for_address(cls, address, timeout=None): |
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.
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
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) |
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.
I tried to make the client a bit smart on handling timeouts when it's going to try many URL variations
@@ -103,6 +103,7 @@ const distSpecFilePath = path.resolve(__dirname, '../dist/apiSpec.json'); | |||
try { | |||
apiSpec = specModule(specFilePath); | |||
} catch (e) { | |||
console.error(e); |
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.
Logging this error helped clarify what was going wrong when confronted with an error about missing apiSpec.json
return false; | ||
}; | ||
} | ||
return 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.
Refactored this for re-use with new device composables. Its change in interface didn't affect existing usage.
513c95d
to
c885a53
Compare
Build Artifacts
|
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.
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
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
c885a53
to
10202d5
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.
Tested and works great in the frontend. I'm looking forward to using it soon.
Thanks!
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 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!
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
Adds new strings for surfacing connection status to the user in the 'Select network device' modalReferences
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
on_my_own_setup
How it works
When you open the 'Select network device' modal:
While the 'Select network device' modal is open:
Some devices are not responding. ...
if any device becomes unavailable (disabled)When the 'Select network device' modal is submitted:needs #10153Screenshots
Modal: at submit
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)