Skip to content
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

Process call recordings and call transcriptions notifications independently #10468

Closed

Conversation

Ivansss
Copy link
Member

@Ivansss Ivansss commented Sep 5, 2023

☑️ Resolves

🏁 Checklist

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Call chains seem to be fine (for recording/transcription), but I'm afraid that we could miss some other notification usecases

@SystemKeeper
Copy link
Contributor

I would assume that the timestamp for a recording and the transcript differs? The notification should then only be processed if the datetime matches:

https://github.com/nextcloud/notifications/blob/9e4bfdcf04d3a17402f4b22166e1afd4fc7ff833/lib/Handler.php#L322-L325

I tested this on a 27.0.2 installation, and it is working correctly for me. These are the notifications generated:

> SELECT app,timestamp,object_type,subject,subject_parameters,message,message_parameters actions FROM oc_notifications WHERE user = 'marcel.mueller';
+--------+------------+-------------+------------------------+---------------------+---------+---------+
| app    | timestamp  | object_type | subject                | subject_parameters  | message | actions |
+--------+------------+-------------+------------------------+---------------------+---------+---------+
| spreed | 1693940282 | recording   | record_file_stored     | {"objectId":850850} |         | []      |
| spreed | 1693940435 | recording   | transcript_file_stored | {"objectId":850851} |         | []      |
+--------+------------+-------------+------------------------+---------------------+---------+---------+

and after I shared the recording, the transcript-notification is still visible:

image

And the code was already there in 17 🤔

@SystemKeeper
Copy link
Contributor

Updated the instance to the latest 27.1 RC.

> SELECT app,timestamp,object_type,subject,subject_parameters,message,message_parameters actions FROM oc_notifications WHERE user = 'marcel.mueller';
+--------+------------+-------------+------------------------+---------------------+---------+---------+
| app    | timestamp  | object_type | subject                | subject_parameters  | message | actions |
+--------+------------+-------------+------------------------+---------------------+---------+---------+
| spreed | 1693941375 | recording   | record_file_stored     | {"objectId":850861} |         | []      |
| spreed | 1693941632 | recording   | transcript_file_stored | {"objectId":850862} |         | []      |
+--------+------------+-------------+------------------------+---------------------+---------+---------+

so the timestamps are different.

I am able to reproduce the issue if I have a second tab open. In this case I get the following log in the console:

Deferring notification refresh from browser storage are notify_push event to give the last tab the chance to do it

When I then open the notification popup and click on the share button, both notifications vanish. After the next notification refresh, the remaining one is back again. So for me, this looks like a frontend issue, not a backend one.

@danxuliu
Copy link
Member

danxuliu commented Sep 6, 2023

So for me, this looks like a frontend issue, not a backend one.

It seems so :-)

@nickvergessen nickvergessen deleted the process-recording-notifications-independently branch September 18, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression 27.1 - notifications disappear
5 participants