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

Set the filename in Content-Disposition for inline files #8010

Closed
wants to merge 1 commit into from

Conversation

jsma
Copy link
Contributor

@jsma jsma commented Feb 17, 2022

In 2.11, the sendfile util was changed to serve PDFs inline. In the process, this prevented the filename from being set so when users go to save the PDF, the filename would be blank in the save dialog. This PR always sends the filename in the "Content-Disposition" header whether the file is served as inline or attachment so the save dialog will have a sensible default value.

@squash-labs
Copy link

squash-labs bot commented Feb 17, 2022

Manage this branch in Squash

Test this branch here: https://greenlightgoset-filename-for-i-rxmo0.squash.io

@lb-
Copy link
Member

lb- commented Feb 22, 2022

@jsma this looks good in principle, but could you please update the unit tests for this + add some for this behaviour.

Additionally, there has been some reformatting work done since this PR was created so you will need to run black to reformat the changes.

@gasman
Copy link
Collaborator

gasman commented Feb 22, 2022

@lb- Unless I'm missing something, I think the existing test here does the job - the change is to add a filename to the header that wasn't there before, and the test has been updated to reflect that.

@lb-
Copy link
Member

lb- commented Feb 22, 2022

@gasman good call, took another look and yeah I see that now.

I haven't validated locally yet and still needs a rebase. Might be able to take a look at that tomorrow but feel free to pick it up if you want.

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

Looks good @jsma I have tested locally and it does what it says on the box - thanks so much!
Screen Shot 2022-02-23 at 6 45 27 am

@lb-
Copy link
Member

lb- commented Feb 22, 2022

Merged in via 4a7fb00

@lb- lb- closed this Feb 22, 2022
@jsma jsma deleted the set-filename-for-inline-files branch February 22, 2022 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants