Skip to content

File uploads#164

Merged
kalilsn merged 7 commits into
mainfrom
gs/file-upload
Nov 16, 2023
Merged

File uploads#164
kalilsn merged 7 commits into
mainfrom
gs/file-upload

Conversation

@gabestein
Copy link
Copy Markdown
Member

@gabestein gabestein commented Nov 8, 2023

Issue(s) Resolved

Oops, forgot to create one.

--> Requires updating core's .env.local file with AWS bucket keys (in our 1Pass).

This adds a (poorly styled!) file uploader using Uppy, as well as some unjournal form enhancements.

There are two outstanding issues I need a little help tracking down:

  • somewhere deep in the stack, Zod is complaining about receiving an array in createPub in the form for the file uploads, despite arrays being a valid record (as far as I know?).
  • Uppy's UI requires importing CSS from node_modules, which seems impossible to do via Tailwind CSS loaders. So I need a little help on a strategy for that.

Anyway, otherwise it works??

Test Plan

  • Create a Pub and invite an evaluator.
  • Upload a file. Many files, even.
  • Submit and see network response including the pubpub:fileUpload formField value with an array of files including public S3 links!

Screenshots (if applicable)

Optional

Notes/Context/Gotchas

  • I could use some double-checking of the permissions settings of the new S3 bucket.
  • I had to upgrade our Cloudflare SSL to serve from assets.v7.pubpub.org!
  • Right now uses the entire upload URL, but it's being proxied to assets.v7.pubpub.org so we could also parse out the key from the returned URL and use that.
  • Right now, all asset links are public, just like in current PubPub assets. Ideally, we'd actually make all asset links private and use another API route to get signed links from core when we need to work with assets. This can be implemented pretty easily in the future.
  • Uses a {pubId}/{fileName} key strategy, so that files are grouped by Pub. This might be a good strategy, but it also might not, because we might ultimately want to create a gallery that reuses assets across pubs? Idk. I don't think it's super important right now, but something to think on a little.

Supporting Docs

@isTravis isTravis requested a deployment to gs/file-upload - core PR #164 November 8, 2023 22:42 — with Render Abandoned
@isTravis isTravis had a problem deploying to gs/file-upload - flock PR #164 November 8, 2023 22:42 — with Render Failure
@isTravis isTravis deployed to gs/file-upload - flock PR #164 November 8, 2023 22:50 — with Render Active
@gabestein gabestein changed the title Gs/file upload File uploads Nov 8, 2023
@isTravis isTravis deployed to gs/file-upload - flock PR #164 November 12, 2023 18:22 — with Render Active
Copy link
Copy Markdown
Collaborator

@3mcd 3mcd left a comment

Choose a reason for hiding this comment

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

I still need to test and provide feedback for the Zod and CSS import issues, but I went ahead and left a little bit of early feedback.

Comment thread core/lib/server/assets.ts Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could use the expect function from "utils" to have a bit more granular error reporting here, like:

const region = expect(process.env.ASSETS_REGION, "ASSETS_REGION environment variable missing")
// ...

Comment thread core/lib/server/assets.ts Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove log statement.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some of our fields use snake-case e.g. "unjournal:evaluated-paper", so we should use that here or convert everything to camelCase. I have no strong feelings about either.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A comment here explaining what fullFormats is doing would be helpful.

@isTravis isTravis had a problem deploying to gs/file-upload - flock PR #164 November 15, 2023 18:57 — with Render Failure
@isTravis isTravis temporarily deployed to gs/file-upload - core PR #164 November 15, 2023 18:57 — with Render Destroyed
@isTravis isTravis deployed to gs/file-upload - flock PR #164 November 15, 2023 18:58 — with Render Active
@isTravis isTravis deployed to gs/file-upload - flock PR #164 November 15, 2023 20:52 — with Render Active
@isTravis isTravis deployed to gs/file-upload - flock PR #164 November 16, 2023 15:23 — with Render Active
@isTravis isTravis temporarily deployed to gs/file-upload - core PR #164 November 16, 2023 15:44 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - integration-evaluations PR #164 November 16, 2023 15:44 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - integration-submissions PR #164 November 16, 2023 15:44 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - flock PR #164 November 16, 2023 15:44 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - flock PR #164 November 16, 2023 15:58 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - core PR #164 November 16, 2023 15:58 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - integration-submissions PR #164 November 16, 2023 15:58 — with Render Destroyed
@isTravis isTravis temporarily deployed to gs/file-upload - integration-evaluations PR #164 November 16, 2023 15:58 — with Render Destroyed
@kalilsn kalilsn merged commit 4edb882 into main Nov 16, 2023
@kalilsn kalilsn deleted the gs/file-upload branch November 16, 2023 16:02
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.

4 participants