search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 724 forks source link

Cannot delete enterprise with products (soft-delete enterprises) #2971

Open sauloperez opened 6 years ago

sauloperez commented 6 years ago

Description

Spree 2.0 made products soft-deletable in https://github.com/spree/spree/commit/3a9bbab32855e94484ae522b7f002867e1a5915b. That means that destroys now become updates. This is why the test below fails in 2-0-stable

https://github.com/openfoodfoundation/openfoodnetwork/blob/50a06d1bd9a88b35381b41e95a90a266a3640264/spec/models/enterprise_spec.rb#L36-L43

Now the product doesn't get deleted from DB, then the enterprise does get deleted and that leaves the product orphan, which causes

PG::ForeignKeyViolation: ERROR:  update or delete on table "enterprises" violates foreign key constraint "spree_products_supplier_id_fk" on table "spree_products"
       DETAIL:  Key (id)=(5) is still referenced from table "spree_products".
       : DELETE FROM "enterprises" WHERE "enterprises"."id" = $1

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 the enterprises table. Then, enable it by calling act_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 with destroy :warning:

kirstenalarsen commented 6 years ago

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?

Matt-Yorkley commented 6 years ago

It shouldn't really affect requirements for deleting data, and yes it will mean we soft-delete enterprises.

mkllnk commented 5 years ago

I made some progress in using the new delete API in https://github.com/openfoodfoundation/openfoodnetwork/pull/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?

sauloperez commented 5 years ago

Totally up for separating this from the upgrade but how do we handle the described case since #3394?

mkllnk commented 5 years ago

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?

luisramos0 commented 5 years ago

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 (note: making delete "enterprise with products AND orders" work is a different topic on top of this one. that is not working in v1 and will not work in v2)

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?

lin-d-hop commented 5 years ago

@luisramos0 Personally I think your approach is acceptable given the current state of things.

kirstenalarsen commented 5 years ago

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?

RachL commented 5 years ago

@luisramos0 I think as well that v2 should be a priority over enterprise proper removal here.

luisramos0 commented 5 years ago

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.

tschumilas commented 5 years ago

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.)

luisramos0 commented 5 years ago

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.

tschumilas commented 5 years ago

what do you mean delete order cycles 'manually' - you mean strip them of suppliers?

luisramos0 commented 5 years ago

there's a delete button in the order cycles list next to each order cycle without orders.

sauloperez commented 5 years ago

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.

luisramos0 commented 5 years ago

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.

tschumilas commented 5 years ago

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.

luisramos0 commented 5 years ago

I think this is the issue representing "fix delete enterprises feature". @sauloperez raised the question on slack, updating here with @mkllnk's useful hint:

The inability to delete enterprises frustrated instance managers a lot in the past. Has been discussed many times. The current solution is:

  • Rename the enterprise, e.g. DELETE - enterprise name
  • Change the owner to a dedicated user owning all enterprises to delete.
  • Remove all managers and permissions.
  • Make the enterprise invisible.
sauloperez commented 4 years ago

A first quick attempt was done in https://github.com/openfoodfoundation/openfoodnetwork/pull/4409. We should check that code whenever we work on this as there's some know-how there that will be useful.

https://github.com/openfoodfoundation/openfoodnetwork/pull/4411 might also come in handy.