Repository navigation
feat(#11281): add an offline sync pairing screen - #11450
paulpascal wants to merge 46 commits into
Conversation
22deadc to
a75d524
Compare
9e3fc1d to
77f6572
Compare
… not work on material
77f6572 to
0d3cfe2
Compare
|
Note 🤖 This is an AI-generated review, requested by @jkuester. Its content and suggestions can be wrong. Please respond with a comment if you have doubts regarding any of the review. A maintainer will also review this PR. 🔍 OpenCodeReview found 24 issue(s) in this PR.
Requirements
Preconditions to confirm
Undisclosed Changes
Alternative Approaches
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| offline_sync.menu.title = Offline sync | ||
| offline_sync.title = Offline sync |
There was a problem hiding this comment.
This file is otherwise maintained in alphabetical key order (e.g. ...number\ records → online.action.message → overwrite.existing.records.help → partner.*... around line 989-992). The new offline_sync.* block (48 keys) is appended at the very end of the file instead of being inserted alphabetically between number\ records and online.action.message, where offline_sync sorts. This breaks the established convention and makes future diffs/merges harder to review, and increases the risk of accidentally introducing duplicate keys that go unnoticed.
| offline_sync.menu.title = Offline sync | ||
| offline_sync.title = Offline sync | ||
| offline_sync.loading = Checking this device... | ||
| offline_sync.unsupported.title = Mobile app required | ||
| offline_sync.unsupported.description = Offline sync requires the CHT Android app. Open this page on your Android device to use it. | ||
| offline_sync.no_role.title = Offline sync not available | ||
| offline_sync.no_role.description = Your user role is not configured for offline sync. Contact your administrator if you need access. | ||
| offline_sync.host.title = Receive data |
There was a problem hiding this comment.
Beyond being appended at the end of the file, the new offline_sync.* keys are also not sorted alphabetically among themselves (menu.title, title, loading, unsupported., no_role., host., wifi., peer., failed., error.* — grouped by UI flow instead). Other multi-key groups in this file (e.g. android_app., contact.profile.) keep their sub-keys in strict alphabetical order. Sorting this block internally (e.g. error.* < failed.* < host.* < loading < menu.title < no_role.* < peer.* < title < unsupported.* < wifi.*) would match the established convention and make future diffs/lookups easier.
Suggestion:
| offline_sync.menu.title = Offline sync | |
| offline_sync.title = Offline sync | |
| offline_sync.loading = Checking this device... | |
| offline_sync.unsupported.title = Mobile app required | |
| offline_sync.unsupported.description = Offline sync requires the CHT Android app. Open this page on your Android device to use it. | |
| offline_sync.no_role.title = Offline sync not available | |
| offline_sync.no_role.description = Your user role is not configured for offline sync. Contact your administrator if you need access. | |
| offline_sync.host.title = Receive data | |
| offline_sync.error.certificate_failed = This device cannot secure the connection. Offline sync will not work on this phone, use another device to receive. | |
| ... (keys sorted alphabetically) ... | |
| offline_sync.wifi.ssid = WiFi network |
| .mat-mdc-button-base { | ||
| margin-right: 10px; | ||
| margin-top: 5px; | ||
| } |
There was a problem hiding this comment.
margin-right is a physical property and isn't swapped by direction: rtl. Other modules in this codebase that use margin-right pair it with a matching override in rtl.less (see .select-mode-available, .item-summary, etc.), but no such override was added for .offline-sync here, so the host/scan/stop/retry buttons will keep right-side spacing even in RTL locales.
There was a problem hiding this comment.
Related: we should be sure to smoke-test the new layout with an RTL language (Arabic) and make sure it renders as expected.
| .offline-sync-scope { | ||
| display: flex; | ||
| flex-direction: column; | ||
| margin-bottom: 10px; | ||
| } |
There was a problem hiding this comment.
.offline-sync-scope isn't referenced anywhere in offline-sync.component.html (or anywhere else in the webapp). This looks like leftover/dead CSS from an earlier version of the template — either wire it up where it's needed or remove it.
| */ | ||
| private async checkOfflineSyncVisibility() { | ||
| try { | ||
| this.offlineSyncVisible = await this.offlineSyncService.canHost() || await this.offlineSyncService.canJoin(); |
There was a problem hiding this comment.
canHost() and canJoin() are independent async checks (each calls AuthService.has(), which hits CHTDatasourceService), but they are awaited sequentially here, so most users (who can't host) pay for two round-trips instead of one. OfflineSyncComponent.ngOnInit() already runs the same two calls via Promise.all([...]) — using the same pattern here would be both faster and consistent with that file.
Suggestion:
| this.offlineSyncVisible = await this.offlineSyncService.canHost() || await this.offlineSyncService.canJoin(); | |
| const [canHost, canJoin] = await Promise.all([ | |
| this.offlineSyncService.canHost(), | |
| this.offlineSyncService.canJoin(), | |
| ]); | |
| this.offlineSyncVisible = canHost || canJoin; |
| private get bridge() { | ||
| const bridge = (globalThis as any)?.medicmobile_android ?? null; |
There was a problem hiding this comment.
medicmobile_android is already declared as any on the global Window interface in polyfills.ts, and other services (app.component.ts, task-notifications.service.ts) access it via globalThis?.medicmobile_android without any cast. The (globalThis as any) cast here is an unnecessary, uncommented any usage that's inconsistent with the rest of the codebase.
Suggestion:
| private get bridge() { | |
| const bridge = (globalThis as any)?.medicmobile_android ?? null; | |
| private get bridge() { | |
| const bridge = globalThis?.medicmobile_android ?? null; |
| isHosting(): boolean { | ||
| return !!this.bridge?.offline_sync_is_hosting(); | ||
| } |
There was a problem hiding this comment.
isHosting() is never called from OfflineSyncComponent or anywhere else in the app (only exercised by its own unit test). As it stands, OfflineSyncComponent.ngOnInit never checks whether a hosting session is already active, so re-entering this screen after navigating away (or recreating the component) while the native side is still hosting will show the 'idle' start-hosting card instead of reflecting the in-progress session. Either use this method to restore state on init, or remove it if it's genuinely unused.
| permissionsResolvedBy(granted: boolean) { | ||
| this.permissionsSubject.next(granted); | ||
| } |
There was a problem hiding this comment.
Naming is inconsistent with the sibling resolver methods on this service: hostingResolved(...) and pairingResolved(...) follow a <noun>Resolved pattern, but this one is permissionsResolvedBy. Consider renaming to permissionsResolved(granted) (and renaming the observable getter above, which already has that name, to something else) for consistency.
| fixture = TestBed.createComponent(OfflineSyncComponent); | ||
| component = fixture.componentInstance; | ||
| await component.ngOnInit(); |
There was a problem hiding this comment.
create() manually invokes and awaits ngOnInit() (needed because it's async), but two tests ('shows the network beside the code...' and 'offers a way back after a failure') later call fixture.detectChanges(). Since detectChanges() has never run on this fixture yet, Angular treats it as the view's first check and invokes ngOnInit() a second time, adding a second subscription to hostingResult()/pairingResult()/permissionsResolved(). In the current tests this is harmless only because no .next() call happens after that second detectChanges(); if a future edit emits on these subjects afterward, handlers (and side effects like feedbackService.submit) would fire twice, silently producing wrong counts. Consider guarding against a second detectChanges()-triggered ngOnInit() (e.g. by not calling detectChanges() in those two tests, or by not manually invoking ngOnInit() and instead awaiting the service promises after a single detectChanges()).
| expect(component.supported).to.be.false; | ||
| expect(component.canHost).to.be.false; | ||
| }); |
There was a problem hiding this comment.
The test configures canJoin to resolve false but never asserts component.canJoin, so this branch is exercised without verification. If the component ever fails to wire the join-availability flag through (e.g. only canHost is read), this test would still pass even though the title claims the screen 'offers nothing'.
Suggestion:
| expect(component.supported).to.be.false; | |
| expect(component.canHost).to.be.false; | |
| }); | |
| expect(component.supported).to.be.false; | |
| expect(component.canHost).to.be.false; | |
| expect(component.canJoin).to.be.false; | |
| }); |
github-actions
Bot
removed
the
ai-review
…ompt as a failure
|
Cc: @inromualdo |
Description
Closes #11281 (webapp half). The android half is medic/cht-android#443.
Adds an
offline-syncpage where one device hosts a session and another joins it by scanning a QR code. The page talks to cht-android only throughOfflineSyncService, over themedicmobile_androidbridge.The entry shows in both navigations (header and sidebar) when the user holds the permission for that side AND the device can do it:
can_relay_offline_data_bundleandoffline_sync_host_available()can_send_offline_data_bundleandoffline_sync_join_available(), never for an online-only user, since the server refuses their bundlesHosting needs Android 8 and joining works on everything we support, so the webapp asks the app instead of guessing from a version. An app older than this webapp has the bridge but not these methods, so it counts as no bridge: the entry stays hidden and the page says "Mobile app required" instead of throwing.
Android answers through three
AndroidApi.v1callbacks:resolveOfflineSyncHostingResult,resolveOfflineSyncPairingandofflineSyncPermissionsResolved. What it sends is always a code, so all the wording lives in the translation files, en es fr ne sw. The Nepali and Swahili strings could use a native speaker's eye.The two permissions are documented in medic/cht-docs#2267 (draft until release).
Code review checklist
can_view_old_navigationpermission to see the old design. Test it has appropriate design for RTL languages.License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.