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

PRO-2191 download #3

Merged
merged 6 commits into from
Nov 8, 2021
Merged

PRO-2191 download #3

merged 6 commits into from
Nov 8, 2021

Conversation

abea
Copy link
Contributor

@abea abea commented Nov 2, 2021

No description provided.

@abea abea changed the title Pro 2227 download PRO-2191 download Nov 4, 2021
@linear
Copy link

linear bot commented Nov 4, 2021

PRO-2191 As an editor, once my export file has been generated it will automatically start downloading for me

This is a front-end task for presenting the generated file to the user to be downloaded.

A separate ticket exists for the actual handling of the export process.

Acceptance criteria:

  • After I've clicked the export button, and the file has been generated, I will automatically be prompted by the browser to download it

See below for a design with the relevant notifications, once the "Export complete" notification is sent, the browser should prompt the user to download the file automatically.

image.png

@apostrophecms apostrophecms deleted a comment from linear bot Nov 4, 2021
@abea abea marked this pull request as ready for review November 4, 2021 16:29
@abea abea requested a review from boutell November 4, 2021 21:01
index.js Show resolved Hide resolved
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

We might need content-disposition here, let me know if I'm wrong. Maybe we don't simply because none of the output formats are native to the browser anyway, but it's a good idea to specify downloading is the intent.

@@ -126,8 +126,26 @@ module.exports = (self) => {
return reject(error);
}

const downloadUrl = self.apos.attachment.uploadfs.getUrl() + downloadPath;
Copy link
Member

Choose a reason for hiding this comment

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

Do you really get a download this way across browsers with just _blank? I would think you would need to use a route that sends with with Content-Disposition: Attachment (I think I sent you the 2.x route we use for it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only one I see that does not download is Safari, which opens the CSV as text in a new tab. This is essentially the way we handled it in the A2 version. https://github.com/apostrophecms/apostrophe-pieces-export/blob/main/public/js/export-modal.js#L39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're saving the file to the public directory I'm not sure how much we get out of putting it behind another route.

@abea abea requested a review from boutell November 5, 2021 16:22
Copy link
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

In the A2 version we used a route to download it and that route emits the right Content-Disposition header, as a result of which I betcha Safari doesn't show it in a tab.

@boutell
Copy link
Member

boutell commented Nov 5, 2021

(In A3 this would need to be in routes because we need to respond directly via res and in A3 apiRoutes just return a body.)

  // Provides a simple route to download a file as an attachment, rather than
  // viewing it. Pipes it from the regular public URL to avoid acting as a
  // bypass of permissions
  self.apiRoute('get', 'download', async (req, res) => {
    const _id = self.apos.launder.id(req.query._id);
    const info = await self.db.findOne({
      _id: _id
    });
    if (!info) {
      return res.status(404).send('notfound');
    }
    const url = self.url(info, { size: 'original' });
    res.setHeader('Content-Disposition', `attachment; filename="${path.basename(url)}"`);
    res.setHeader('Content-Type', 'application/octet-stream');
    const resolvedUrl = (new URL(url, req.absoluteUrl)).toString();
    await request(resolvedUrl).pipe(res);
  });

@boutell
Copy link
Member

boutell commented Nov 5, 2021

In A2 that route is in core, as there can be other applications for reliably downloading rather than viewing an attachment, which I think is still the case.

Base automatically changed from PRO-2189-modal to main November 5, 2021 18:46
@abea
Copy link
Contributor Author

abea commented Nov 5, 2021

This is essentially the way we handled it in the A2 version.

By that I meant it's how it was done in the pieces export module.

@abea abea requested a review from boutell November 5, 2021 19:06
@abea abea merged commit 893e690 into main Nov 8, 2021
@abea abea deleted the PRO-2227-download branch November 8, 2021 16:49
This pull request was closed.
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.

2 participants