[OGUI-1854] Notify user via native browser notification when a new run has started #3258
Conversation
…` in `header.js`
…` in `header.js`
hehoon
force-pushed
the
feature/QCG/OGUI-1854/notify-user-when-a-new-run-has-started
branch
from
caf1e3f to
db1b8f2
Compare
…arted' of github.com:AliceO2Group/WebUi into feature/QCG/OGUI-1854/notify-user-when-a-new-run-has-started
| this._ongoingRuns.delete(runNumber); | ||
| } | ||
|
|
||
| const wsMessage = new WebSocketMessage(); |
There was a problem hiding this comment.
Each event sent by Kafka on the run track has multiple transitions. For the notification of the user, we should only send one broadcast message when we receive the event of START_ACTIVITY: L127
Otherwise, we risk sending up to 10 messages for a run start at various intervals of seconds
This information should have been provided in the ticket but it was not, I apologise for the confusion
| bookkeepingService, | ||
| dataService, | ||
| eventEmitter, | ||
| ws, |
There was a problem hiding this comment.
As this is passed as a parameter, could you please use a more descriptive naming? Perhaps webSocketService or something along those lines
| * @returns {undefined} | ||
| */ | ||
| async _handleWSRunTrack({ runNumber, transition }) { | ||
| if (transition !== Transition.START_ACTIVITY) { |
There was a problem hiding this comment.
With the backend change to only notify on start activity, this check can be removed
| let permissionGranted = false; | ||
| if (event.target.checked) { | ||
| const permission = await requestBrowserNotificationPermissions(); | ||
| permissionGranted = permission === BrowserNotificationPermission.GRANTED; |
There was a problem hiding this comment.
What about using the framework's method areBrowserNotificationsGranted instead?
| model.session.personid === 0 // Anonymous user has id 0 | ||
| ? h('p.m3.gray-darker', 'This instance of the application does not require authentication.') | ||
| : h('a.menu-item', { onclick: () => alert('Not implemented') }, 'Logout'), | ||
| h( |
There was a problem hiding this comment.
It would be great to extract this into its own documented component which should recieve only the needed model notificationRunStartModel
| onclick: () => { | ||
| const { isRunModeActivated } = this.model.filterModel; | ||
| if (!isRunModeActivated) { | ||
| const viewModel = this.model.filterModel.getPageTargetModel(); | ||
| if (viewModel) { | ||
| this.model.filterModel.activateRunsMode(viewModel); | ||
| } | ||
| } | ||
|
|
||
| this.model.filterModel.setFilterValue('RunNumber', runNumber?.toString(), true); |
There was a problem hiding this comment.
I believe this method's logic can be improved and I should have provided more details in the requirements.
Basically, when such a notification is provided the user can either be:
- In RunMode of a previous RunNumber (for sure it cannot be in the current one)
- in this case, the method should update the run mode with new runNumber
- Outside of runMode
a. If Kafka is configured, then the method should activate run mode with new run number
b. If Kafka is not configured, the method simply update the Filter RunNumber parameter with new run number and trigger the filtering
In all scenarios, the user should be taken back to objectTree page no matter where they were
There was a problem hiding this comment.
This has been adjusted to support scenario 1 and 2a 👍 .
To receive notifications from the websocket kafka must be enabled, so I believe scenario 2b cannot occur. Do let me know if I missed something.
| * are enabled for the current user. | ||
| * @returns {boolean} `true` if notifications are enabled, `false` otherwise. | ||
| */ | ||
| getBrowserNotificationSetting() { |
There was a problem hiding this comment.
Should this method not take into account the browser's permissions?
I am thinking of the following use case:
- user enables notifications via QCG which in turns saves it as true in the local storage
- at some point they disable the notifications via the browser instead. At this point, QCG still thinks notifications are enabled and will be sent but instead they will not be.
There was a problem hiding this comment.
Good catch, this has been adjusted 👍
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
I have JIRA issue created
Depends on:
Changes: