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

Make invoice file separate field #10702

Merged
merged 4 commits into from
Feb 25, 2025
Merged

Conversation

hdiniz
Copy link
Contributor

@hdiniz hdiniz commented Feb 13, 2025

Related: opencollective/opencollective#7726

Add invoice expense attachments as its own field on the expense object, differentiating from other attachment types.

@hdiniz hdiniz self-assigned this Feb 13, 2025
@hdiniz hdiniz force-pushed the make-invoice-file-separate-field branch from c8eac2a to db35082 Compare February 13, 2025 14:03
@hdiniz hdiniz force-pushed the make-invoice-file-separate-field branch 2 times, most recently from e50883a to ca22e29 Compare February 20, 2025 10:03
@hdiniz hdiniz marked this pull request as ready for review February 20, 2025 10:39
@hdiniz hdiniz requested review from gustavlrsn and Betree February 20, 2025 10:39
Copy link
Member

@gustavlrsn gustavlrsn left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@gustavlrsn gustavlrsn left a comment

Choose a reason for hiding this comment

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

Getting this error when used with the mock uploads locally:

Screenshot 2025-02-24 at 12 38 29 Screenshot 2025-02-24 at 12 38 19

@hdiniz
Copy link
Contributor Author

hdiniz commented Feb 24, 2025

Getting this error when used with the mock uploads locally:
Screenshot 2025-02-24 at 12 38 29 Screenshot 2025-02-24 at 12 38 19

This only works without mocked uploads, we should be deprecating that asap. I added a few changes here to use minio on e2e tests for that reason. I will try to remove it entirely.

@hdiniz hdiniz force-pushed the make-invoice-file-separate-field branch from ca22e29 to bf5fa88 Compare February 24, 2025 20:09
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.

The context of this change is not clear, can we link a GitHub issue or add a description?

@@ -58,6 +66,14 @@ jobs:
- 5432:5432
# needed because the postgres container does not provide a healthcheck
options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
minio:
image: minio/minio:edge-cicd
Copy link
Member

Choose a reason for hiding this comment

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

I initially wanted to use this image as well in #10688, but it hasn't been updated in 3 years and that was causing some issues in my tests (I don't remember where), which is why I went with the npm run minio & approach.

No blocker for me if the image is working here, we can always update later if needed.

@hdiniz hdiniz merged commit 1cea497 into main Feb 25, 2025
20 checks passed
@hdiniz hdiniz deleted the make-invoice-file-separate-field branch February 25, 2025 15:12
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.

3 participants