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
[mod_export] Observe content disposition #1847
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for the change in general. Can you fix some details before we merge this?
], | ||
{Bin, _Context} = z_template:render_to_iolist({cat, "atom/entry.tpl"}, Vars, Context), | ||
{ok, iolist_to_binary(Bin), State}; | ||
case m_rsc:p(Row, is_published, Context) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mworrell Should we add an ACL check here? If so, it should be is_visible
.
undefined -> | ||
Cat = m_rsc:p_no_acl(Id, category, Context), | ||
Filename = "export-" | ||
Filename2 = "export-" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Filename
variable should be moved before the case
:
Filename = case z_notifier:first(#export_resource_filename{id=Id, dispatch=Dispatch, content_type=ContentType}, Context) of
undefined ->
...
LGTM now. @mworrell Please have a look and squash & merge if okay. |
Rss feeds are currently handled by browsers as a download while the Rss reader plugins want Content-Disposition inline.
This PR allows Content-Disposition to be inline instead of attachment.
It also fixes a problem with unpublished resources to show up in feeds.