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.

Sign up for GitHub

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

Jump to bottom

Provide signed url for uploaded expense files #10500

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Provide signed url for uploaded expense files #10500

wants to merge 32 commits into from

Conversation

hdiniz
Copy link
Contributor

@hdiniz hdiniz commented Nov 27, 2024

@hdiniz hdiniz self-assigned this Nov 27, 2024
@hdiniz hdiniz force-pushed the provide-signed-url-for-uploaded-expense-files branch from 994d54c to 001ee3c Compare November 28, 2024 19:57
@hdiniz hdiniz mentioned this pull request Nov 28, 2024
Open
@hdiniz hdiniz requested a review from Betree November 28, 2024 20:32
@hdiniz hdiniz marked this pull request as ready for review November 28, 2024 20:32
Betree
Betree reviewed Nov 29, 2024
View reviewed changes
Copy link
Member

@Betree Betree left a comment

Choose a reason for hiding this comment

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

Since it's serving a critical feature with sensitive permissions, we need to add proper tests for the /files/... controller.

migrations/20241127165812-add-expense-attachments-url-indexes.js Outdated Show resolved Hide resolved
server/lib/awsS3.ts Show resolved Hide resolved
server/lib/utils.js Show resolved Hide resolved
server/models/UploadedFile.ts Outdated Show resolved Hide resolved
server/models/UploadedFile.ts Outdated Show resolved Hide resolved
server/models/UploadedFile.ts Outdated Show resolved Hide resolved
server/routes.js Outdated Show resolved Hide resolved
templates/partials/expense-items.hbs Outdated Show resolved Hide resolved
server/controllers/files.ts Outdated Show resolved Hide resolved
server/controllers/files.ts Outdated Show resolved Hide resolved
@hdiniz hdiniz requested a review from Betree November 29, 2024 19:43
Betree
Betree reviewed Dec 2, 2024
View reviewed changes
server/controllers/files.ts Outdated Show resolved Hide resolved
@Betree Betree mentioned this pull request Dec 2, 2024
Open
@hdiniz hdiniz requested a review from Betree December 4, 2024 12:02
Betree
Betree reviewed Dec 5, 2024
View reviewed changes
server/graphql/common/expenses.ts Outdated Show resolved Hide resolved
templates/partials/expense-items.hbs Outdated
@@ -1,3 +1,15 @@
<style>
.preview {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if classes are scoped in our email setup (we're using juice to inline CSS). If not, maybe use something that is less likely to clash (e.g. expense-item-preview)

package.json Show resolved Hide resolved
@hdiniz
Copy link
Contributor Author

hdiniz commented Dec 5, 2024

Thanks for your review on this @Betree, I have made changes according to our discussion so far, but its still not complete.
One missing piece here is a strategy for dealing with uploaded files used in expense invites, I will look into it next.

hdiniz and others added 18 commits December 11, 2024 16:31
@hdiniz
Add presigned url dependency
d465818
@hdiniz
Add method to presign s3 get request
d51c273
@hdiniz
Add protect url to models
18a0615
@hdiniz
Allow cookie auth on api
abcca87
@hdiniz
Add route to generate presigned url
d2a4598
@hdiniz
Use thumbnail on expense item template partial
e7e90f4
@hdiniz
Fix permission check
b282821
@hdiniz
Fix tests
90ae23a
@hdiniz
Add private cache-control directive
9255b1f
@hdiniz
Add node http request mocks packages
da18c73
@hdiniz
Add test coverage to files controller
83403fc
@hdiniz @Betree
Update server/models/UploadedFile.ts
10105e8 
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
@hdiniz @Betree
Update server/models/UploadedFile.ts
c662007 
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
@hdiniz @Betree
Update server/models/UploadedFile.ts
288eabd 
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
@hdiniz @Betree
Update server/controllers/files.ts
e6653e6 
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
@hdiniz
Fix email template preview icon
2c57f2b
@hdiniz
Add deleted at to expense items url index
6d402d8
@hdiniz
Use uploaded file id in files.getFile controller
1771a98
@hdiniz hdiniz force-pushed the provide-signed-url-for-uploaded-expense-files branch from 2ad64d9 to 8a9d0e0 Compare December 11, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Betree Betree Betree left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@hdiniz hdiniz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hdiniz @Betree