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
Cannot delete enterprise with products (soft-delete enterprises) #2971
Comments
This will get solved once we complete openfoodfoundation#2971. Until then, let's not focus on this spec to reach our 100% green test suite.
could we check this with @myriamboure - is it going to mean that we get closer or further from requirements to handle deleting things? And if Paranoia makes soft deletes easier, will that mean we can actually soft delete enterprises - omg please say yes? |
It shouldn't really affect requirements for deleting data, and yes it will mean we soft-delete enterprises. |
I made some progress in using the new delete API in #3394. It makes the related specs pass. We haven't implemented soft-deleting enterprises though. Can we deal with that as a separate feature not part of the Spree upgrade? |
Totally up for separating this from the upgrade but how do we handle the described case since #3394? |
I think #3394 deals with all the existing delete/destroy calls. That should be all that's required for the Spree upgrade. I think soft-deleting enterprises is an additional feature that is completely independent. @luisramos0 What do you think? |
In v1, we cant delete an "enterprise with products and orders", but we can delete "enterprises with products but NO orders". That's what this spec is testing. the spec is broken because in v2, currently, we cant delete "enterprises with products but no orders". @kirstenalarsen @myriamboure do you think you should (option 1) delay v2 and make sure that we can delete "enterprises with products but NO orders" as we can in v1 or (option 2) should we move forward with v2 and ignore this problem I am not sure what's the best decision here but my vote always goes for getting v2 out there faster, so I'd ignore this in v2.0.0 (option 2) and move this issue outside the upgrade. Is this acceptable or do we need to go for option 1? |
@luisramos0 Personally I think your approach is acceptable given the current state of things. |
Interesting! So you're saying we can delete enterprises with no orders now? How? And if we want to do so we need to do so before V2 is merged?! But given I didn't know that, and is more important to be able to delete both I'd rather not delay V2 . . Or at least not delay much, until we've deleted a bunch of enterprises! Can i do it as superadmin or need to be sysadmin @luisramos? |
@luisramos0 I think as well that v2 should be a priority over enterprise proper removal here. |
ah, good point @kirstenalarsen yes, you can delete them before v2 is released! but only if there are no order cycles and no orders in the enterprise, ok? you can try as super admin, if it fails it's because there are orders or OCs or some other entity attached to the enterprise. everyone seems to agree, I am moving this to all the things as an S3 bug. |
In this case, I want to delete as many old enterprises as I can before v2. No idea that was possible. You are saying I should be able to delete as super admin if there is no order that involves the enterprise's products. Will it work to cancel orders, then delete ? (I'm not talking about 'real' orders - I'm talking about crud accumulating from experiments.) About how long do I have until v2. (This could take a while.) |
cancel orders will not help. any order counts. and I think you will need to delete order cycles manually first if they exist. if it fails you may want to try to delete all data associated with the enterprise like removing connection to ship methods and payment methods as well as deleting permissions. |
what do you mean delete order cycles 'manually' - you mean strip them of suppliers? |
there's a delete button in the order cycles list next to each order cycle without orders. |
Agree on not delaying v2 any further. Keep in mind deleting an OC only works if no one accessed the shop. If you do, an order in cart state attached to the OC gets created and so for the same reason, can't be deleted unless you delete the order first. I managed to do so from the rails console, if it helps. |
good point @sauloperez to check if there are orders in cart state in a OC you can use the orders list page and uncheck the "show completed orders only" checkbox. |
Very helpful - couldn't figure out why I couldn't delete OCs with no orders - and its because they have orders in cart state. (so really, very few OCs can be deleted - because even most of the experimental ones were created so that someone could see how it worked - and so very likely accessing the shop was part of their experimenting). So then, very few enterprises can be deleted also. Too bad. |
I think this is the issue representing "fix delete enterprises feature".
|
Description
Spree 2.0 made products soft-deletable in spree/spree@3a9bbab. That means that destroys now become updates. This is why the test below fails in
2-0-stable
openfoodnetwork/spec/models/enterprise_spec.rb
Lines 36 to 43 in 50a06d1
Now the product doesn't get deleted from DB, then the enterprise does get deleted and that leaves the product orphan, which causes
The solution here is to make enterprises soft-deletable as well by means of paranoia.
Expected Behavior
When deleting an enterprise, its supplied products should be marked as deleted as well as the enterprise itself. None of them will then be visible to users. This should make
spec/models/enterprise_spec.rb:36
pass.Actual Behavior
spec/models/enterprise_spec.rb:36
fails as shown above.Possible Fix
We need to add paranoia in our Gemfile and then run the migration to add the
deleted_at
column to theenterprises
table. Then, enable it by callingact_as_paranoid
.We should watch for calls to⚠️
delete
as these will skip Paranoia's logic and thus get completely deleted from DB. They will need to be replaced withdestroy
The text was updated successfully, but these errors were encountered: