1320: fix deletion of reused files #1696
Conversation
kkoehn
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1696 +/- ##
==========================================
+ Coverage 94.79% 94.87% +0.07%
==========================================
Files 133 133
Lines 3345 3374 +29
==========================================
+ Hits 3171 3201 +30
+ Misses 174 173 -1 ☔ View full report in Codecov by Sentry. |
|
I've spent further time debugging and looking for a solution. Finally, I wasn't able to do so with the current architecture and, hence, need to suggest a major architectural change: Could you try to perform the necessary changes directly by updating and saving the changes (either with the Of course, this means the service will return an altered object, persisted object already and not a modified version (that needs to be save). However, the big advantage is that we can more fine granular control which object gets saved how and don't need to wrap around the Rails magic. Then, you should wrap the I am rather confident that we will be able to navigate around the current issue with this approach, even if it changes the API. Hence, I would suggest to follow with this approach. |
kkoehn
force-pushed
the
1320-fix-destroying-files-while-overwriting
branch
6 times, most recently
from
8fcdde2 to
34309fd
Compare
…r file-consistency
kkoehn
force-pushed
the
1320-fix-destroying-files-while-overwriting
branch
from
34309fd to
cc487bd
Compare
There was a problem hiding this comment.
In an effort to ease domain-specific review, I have executed the code locally and performed my well-known test with the previously-shared set of new and old exercise files (for ID 845). It worked flawlessly and as expected, thank you.
Still, I identified an unnecessary N+1 query caused by the autosave option. It might no longer be necessary (at least in my tests, everything worked without the option, too). Hence, we might be able to drop it:
Subject: [PATCH] Remove autosave causing unnecessary N+1 queries
---
Index: app/models/task_file.rb
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/models/task_file.rb b/app/models/task_file.rb
--- a/app/models/task_file.rb (revision cc487bdb571f5ce4cda4cf42132a76f85e72ef11)
+++ b/app/models/task_file.rb (date 1737201720489)
@@ -4,7 +4,7 @@
include ParentValidation
include TransferValues
- belongs_to :fileable, polymorphic: true, autosave: true, inverse_of: :files
+ belongs_to :fileable, polymorphic: true, inverse_of: :files
belongs_to :parent, class_name: 'TaskFile', optional: true
has_one_attached :attachment
validates :name, presence: true
Index: app/models/model_solution.rb
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/models/model_solution.rb b/app/models/model_solution.rb
--- a/app/models/model_solution.rb (revision cc487bdb571f5ce4cda4cf42132a76f85e72ef11)
+++ b/app/models/model_solution.rb (date 1737201720486)
@@ -5,7 +5,7 @@
include TransferValues
include ParentValidation
- belongs_to :task, autosave: true, inverse_of: :model_solutions
+ belongs_to :task, inverse_of: :model_solutions
belongs_to :parent, class_name: 'ModelSolution', optional: true
has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy
accepts_nested_attributes_for :files, allow_destroy: true
Index: app/models/test.rb
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/models/test.rb b/app/models/test.rb
--- a/app/models/test.rb (revision cc487bdb571f5ce4cda4cf42132a76f85e72ef11)
+++ b/app/models/test.rb (date 1737201720494)
@@ -5,7 +5,7 @@
include TransferValues
include ParentValidation
- belongs_to :task, autosave: true, inverse_of: :tests
+ belongs_to :task, inverse_of: :tests
belongs_to :testing_framework, optional: true
belongs_to :parent, class_name: 'Test', optional: true
validates :title, presence: true
Index: app/models/concerns/file_concern.rb
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/app/models/concerns/file_concern.rb b/app/models/concerns/file_concern.rb
--- a/app/models/concerns/file_concern.rb (revision cc487bdb571f5ce4cda4cf42132a76f85e72ef11)
+++ b/app/models/concerns/file_concern.rb (date 1737201720483)
@@ -4,7 +4,7 @@
extend ActiveSupport::Concern
included do
- has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy, autosave: true, inverse_of: :fileable
+ has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy, inverse_of: :fileable
accepts_nested_attributes_for :files, allow_destroy: true
endPlease keep in mind that I haven't reviewed the code 😇.
unsolved problem of deleting reused files, when parent gets deletedThis PR continues to solve a problem that occurred when file referenced changed for reimported tasks (file belonged to a
testbefore, but now it belongs to amodel_solution).The particular problem here was that the
testreferenced before got deleted. Even when first moving the file and the deleting the test, the file got deleted.see problem description in #1320