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

1037 migrate invoice attachments to active storage slow performance financial links #1039

Conversation

yksflip
Copy link
Member

@yksflip yksflip commented Jan 26, 2024

Until now invoice attachments were stored in the database directly, this led to performance issues see #1037.
This PR migrates the invoice attachments to use active storage. Additionally multiple attachments are now supported, a small preview is rendered and the delete flow was adapted.

fixes #1037

Screenshot from 2024-02-10 17-06-02
Screenshot from 2024-02-10 17-05-41
Screenshot from 2024-02-10 17-05-50

@yksflip yksflip force-pushed the 1037-migrate-invoice-attachments-to-active-storage-slow-performance-financial-links branch from 556f0f6 to 65c0fda Compare February 10, 2024 15:07
until now invoice attachments were stored in the database directly,
this led to performance issues see #1037.
This commit migrates the invoice attachments to use active storage.
Additionally multiple attachments are now supported, a small preview
is rendered and the delete flow was adapted.
@yksflip yksflip force-pushed the 1037-migrate-invoice-attachments-to-active-storage-slow-performance-financial-links branch from 65c0fda to 0bd6048 Compare February 10, 2024 15:30
@yksflip yksflip self-assigned this Feb 10, 2024
@yksflip yksflip marked this pull request as ready for review February 10, 2024 15:45
@yksflip yksflip changed the title wip: 1037 migrate invoice attachments to active storage slow performance financial links 1037 migrate invoice attachments to active storage slow performance financial links Feb 10, 2024
@yksflip yksflip added this to the 4.8.1 milestone Feb 10, 2024
@lentschi
Copy link
Contributor

@yksflip Code looks good to me 👍
Though I do have to admit I haven't tried it out and I wonder why #1037 is happening in the first place:
The fact that attachments are stored in the same db table alone shouldn't cause any performance issues, should it? Maybe this could have been solved by adding attachment_data to the ignored_columns and only load that field whenever we really want to deliver the file contents...?

Anyhow, moving it to active storage is much cleaner 👍

@yksflip
Copy link
Member Author

yksflip commented Feb 11, 2024

thx for reviewing! Ah I see, yeah maybe I concluded a bit too quickly, that might have been enough to resolve the issue.

@yksflip yksflip merged commit 0ff74e8 into master Feb 11, 2024
5 checks passed
@yksflip yksflip deleted the 1037-migrate-invoice-attachments-to-active-storage-slow-performance-financial-links branch February 11, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

migrate invoice attachments to active storage (slow performance financial links)
2 participants