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

feature (api): Add preUploadedPresentation param to API's /create via GET #18953

Merged
merged 5 commits into from Oct 19, 2023

Conversation

GuiLeme
Copy link
Collaborator

@GuiLeme GuiLeme commented Oct 17, 2023

What does this PR do?

It basically implements the pre-upload presentation via GET, so now it is possible to pass a download URL to the create endpoint.

The behavior now is: if one passes in preUploadedPresentation, then this presentation will override the default.pdf, since preUploadedPresentationOverrideDefault=true by default in the application.properties. To avoid that, it is necessary to also send in preUploadedPresentationOverrideDefault=false.

Closes Issue(s)

Closes #18924

More

Notice that all requirements of precedence are respected.

Naming discussion:
One of the main concerns when doing this feature was how to name the presentation sent in the parameter, when the only thing we have is the URL to access it. It can be of the form:

<host>/presentation-name.pdf
<host>/presentation-name.pdf?query1=1
<host>/simply-a-hash-here (just like a shortened link)

So the idea is we are going to let the user specify the presentation's name with a new parameter preUploadedPresentationName, and that will have the priority.

If the name is not specified, it will enter a flow to deduce it from the URL provided.

For the first 2 cases mentioned previously (the examples I gave for the URLs), the links have the extension plugged in, so it is pretty straight forward, the FilenameUtils will extract the name out (with it's logic) and done.

But for the last case, we will consider that if there is no extension explicitly written in the URL, that URL doesn't have the filename in it. And therefore we have no clue what to guess (once we do not want to request the link to see the header, or else we would have to do some more validations due to possible security issues). With that in mind, we will simply name the presentation untitled.<extension> and the extension will be guessed further in the flow based on the mimetype.

@gustavotrott gustavotrott changed the title Feature: GET create with PresentationURL feature (api): Add preUploadedPresentation param to API's /create via GET Oct 17, 2023
@gustavotrott
Copy link
Collaborator

Looks like preUploadedPresentationOverrideDefault=false is not working properly with this param:

override-not-working.no-audio.mp4

@gustavotrott
Copy link
Collaborator

It overrides the default.pdf even when the URL is invalid.
Maybe this is the expected behavior, I'm just reporting to confirm if this is intentional.

invalid-url.no-audio.mp4

@gustavotrott
Copy link
Collaborator

Even when I set preUploadedPresentationOverrideDefault=false, an invalid presentation overrides the default.pdf.
I this case for some reason the default.pdf is being being showed in the Action Button to be selected:

invalid-url-override.no-audio.mp4

@gustavotrott
Copy link
Collaborator

O tried to upload a shortened URL but it didn't work, and I didn't see any error in bbb-web logs:
preUploadedPresentation=https://shorturl.at/cRZ25

Whereas this link used to work in the Pre Uploaded Presentation via POST request.

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!

@sonarcloud
Copy link

sonarcloud bot commented Oct 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@antobinary antobinary merged commit 33f44e1 into bigbluebutton:v2.7.x-release Oct 19, 2023
16 checks passed
@github-actions
Copy link

Automated tests Summary

🚨 Test workflow has failed


Click here to check the action test reports

@antobinary
Copy link
Member

Added an example in the 2.7 docs: #18990

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