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

refactor (graphql-server): Add presentation upload status data to Postgres #18567

Merged

Conversation

paultrudel
Copy link
Collaborator

What does this PR do?

During the presentation upload process data related to the total number of pages being uploaded and the current number of uploaded pages is sent from bbb-web to akka-apps on to the client which is used to show the presentation upload status toast window. This information has been added to Postgres so that the client can access this information through GraphQL instead of being sent the information through an event.

@antobinary antobinary added this to the Release 2.8 milestone Aug 16, 2023
@gustavotrott gustavotrott self-requested a review August 22, 2023 11:56
@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@gustavotrott
Copy link
Collaborator

It's working well!

I just suggest to add a little more information, cause for now we can't show this status:

image
Maybe we could insert the presentation to Db earlier.. on PresentationConversionRequestReceivedSysMsg or PresentationConversionUpdateSysPubMsg.

And it would be good if we could store the error status and msg as well:

image

@paultrudel paultrudel marked this pull request as draft September 6, 2023 19:10
@paultrudel paultrudel marked this pull request as ready for review September 8, 2023 18:38
@gustavotrott
Copy link
Collaborator

Looks like the msg PresentationUploadedFileTimeoutErrorSysPubMsg set the error, but after a few seconds the msg PresentationPageConvertedSysMsg clear the error.

Take a look at second 00:38:

error-msg-cleared.mp4

Moreover: The flag converting keeps true forever and it occurs some error.

@gustavotrott
Copy link
Collaborator

Maybe it is a good idea to create a flag converted in the table pres_page.
Then the message PresentationPageConvertedSysMsg could update directly the page table instead of the presentation.

In this case the prop pagesUploaded would be calculated in a view, instead of having this column fixed in the table pres_presentation.

@paultrudel paultrudel marked this pull request as draft September 12, 2023 11:59
@paultrudel paultrudel marked this pull request as ready for review September 14, 2023 15:55
@gustavotrott
Copy link
Collaborator

It is not storing presentations with error type INVALID_MIME_TYPE, whereas mongodb stores this information:

graphql-doesnt-store-pres-mime-error.no-audio.mp4

It's also missing some extra information to be able to render the error message, for instance:
INVALID_MIME_TYPE_KEY -> fileMime, fileExtension
CONVERSION_TIMEOUT_KEY -> maxNumberOfAttempts, numberPageError
They are all listed in https://github.com/bigbluebutton/bigbluebutton/blob/bc2f1057793cef48a98512408a0325b1850605f0/bigbluebutton-html5/imports/api/presentations/server/handlers/presentationConversionUpdate.js

I suggest to create a generic TEXT column to store this extra info about the error, just like we have to store URLS:
https://github.com/bigbluebutton/bigbluebutton/blob/bc2f1057793cef48a98512408a0325b1850605f0/akka-bbb-apps/src/main/scala/org/bigbluebutton/core/db/PresPresentationDAO.scala#L54C19-L54C19

@paultrudel paultrudel marked this pull request as draft September 19, 2023 14:51
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 23 Code Smells

No Coverage information No Coverage information
1.4% 1.4% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@github-actions
Copy link

Automated tests Summary

🚨 Test workflow has failed


Click here to check the action test reports

@paultrudel paultrudel marked this pull request as ready for review September 21, 2023 15:57
Copy link
Collaborator

@gustavotrott gustavotrott left a comment

Choose a reason for hiding this comment

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

lgtm

@gustavotrott gustavotrott changed the title refactor (graphql): Add presentation upload status data to Postgres refactor (graphql-server): Add presentation upload status data to Postgres Sep 21, 2023
@gustavotrott gustavotrott merged commit fc2b78a into bigbluebutton:develop Sep 21, 2023
20 of 26 checks passed
@danimo danimo mentioned this pull request Nov 28, 2023
1 task
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.

None yet

3 participants