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

Remove invalid upload files #11851

Draft
wants to merge 31 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
43a5fc5
Remove orphaned blobs
alecslupu Oct 27, 2023
5d6ab94
Add Rake task to cleanup old unattached blobs
alecslupu Oct 27, 2023
bd7c937
Refactor task
alecslupu Oct 27, 2023
dcdba03
Add job work to remove unattached blobs after a period of time
alecslupu Nov 8, 2023
87ec9b4
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Nov 8, 2023
f6323cc
Running linters
alecslupu Nov 8, 2023
1f0d755
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Nov 10, 2023
d01bfce
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Mar 8, 2024
84af5a6
Running linters
alecslupu Mar 8, 2024
b51842c
Fix spec
alecslupu Mar 8, 2024
e83826a
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Mar 11, 2024
b9ace94
Fix spelling
alecslupu Mar 11, 2024
dd16d30
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Apr 15, 2024
342751e
Remove the "decidim_core.clean_attachments" initializer and the Clean…
alecslupu Apr 15, 2024
3490ff8
Change the rake task so it prevents deleting blobs that are less than…
alecslupu Apr 15, 2024
63586fa
Change the Releases Notes so the task is run two times with one hour …
alecslupu Apr 15, 2024
2f38d9c
DirectUploadsController validations
alecslupu Apr 16, 2024
7bccc7e
Add specs
alecslupu Apr 16, 2024
2c961cd
Update RELEASE_NOTES.md
alecslupu Apr 16, 2024
8a9342b
Merge branch 'develop' into fix/attachment-validation-cleanup
alecslupu Apr 16, 2024
a544e43
Apply review recommendation
alecslupu Apr 16, 2024
31a384b
Fix specs
alecslupu Apr 17, 2024
dd28ea8
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Jun 9, 2024
e03b5ba
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Jun 12, 2024
74724a2
Convert Blob field to Blob type
alecslupu Jun 12, 2024
7ea1bbe
Remove CSV from allowed types
alecslupu Jun 12, 2024
ee2f2aa
A bit more security
alecslupu Jun 12, 2024
edc60c1
Fix specs
alecslupu Jun 13, 2024
b89acbd
More fixes
alecslupu Jun 13, 2024
5fd8c00
Fix Validator upload
alecslupu Jun 13, 2024
0aa5de1
lint
alecslupu Jun 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion decidim-core/app/commands/decidim/validate_upload.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,19 @@ def initialize(form)
end

def call
return broadcast(:invalid, @form.errors) if @form.invalid?
if @form.invalid?
remove_invalid_file
return broadcast(:invalid, @form.errors)
end

broadcast(:ok)
end

private

def remove_invalid_file
blob = ActiveStorage::Blob.find_signed(@form.blob)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the type of the :blob attribute on the form itself to Decidim::Attributes::Blob.

It does exactly this.

This is probably an oversight in the previous implementation (or the attribute didn't exist back then).

Copy link
Contributor

Choose a reason for hiding this comment

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

@alecslupu Is there some problem implementing this change? I was just wondering because the other changes seem to be already implemented, just this one is still open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahukkanen, there is no issue. Just this PR has not been under my attention lately. also the implementation needs to be changed so that the upload flow to handle the validation as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thanks and noted. Let me know if this still needs my attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 74724a2

blob.purge if blob.present?
end
end
end
13 changes: 13 additions & 0 deletions decidim-core/spec/commands/decidim/validate_upload_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,38 @@ module Decidim
let(:form) do
double(
invalid?: invalid,
blob:,
errors:
)
end
let(:invalid) { false }
let(:errors) { [] }
let(:blob) { "foobar" }

describe "when form is valid" do
it "broadcasts ok" do
expect { command.call }.to broadcast(:ok)
end

it "ignores file removal" do
expect(ActiveStorage::Blob).not_to receive(:find_signed).with(blob)
command.call
end
end

describe "when the form is not valid" do
let(:invalid) { true }
let(:errors) { ["File too dummy"] }

it "broadcasts invalid" do
allow(ActiveStorage::Blob).to receive(:find_signed).with(blob).and_return(double(purge: true))
expect { command.call }.to broadcast(:invalid, errors)
end

it "removes the invalid file" do
expect(ActiveStorage::Blob).to receive(:find_signed)
command.call
end
end
end
end
Expand Down