Openfoodnetwork: Delete a customer from your customer list impossible

[myriamboure]

Description

As a manager of a private shop I try to delete a customer from the customer list to remove access to my shop. The system prevents me to delete it, it says there are orders associated, even if the user has never accessed the shopfront ... and even if they were, as a hub manager I need to be able to remove access to my private shop to a current customer.

Expected Behavior

As a manager of a "distributor" enterprise setup as "private" (= shop only accessible to customers in the customer list of the enterprise), I should be able to remove access to my private shop to some of my current customers.

Actual Behavior

I can't remove access to my private shop to a current customer.

Steps to Reproduce

1. Make sure your enterprise is selling, and is "private" (on shop preference)
2. Check that the shop url leads to a page where it says this shop is only for members (something like that) and you have to login to access the shop. If you are logged in but not in the customer list, you see a message saying you don't have access and need to contact shop manager.
3. Add a customer to your customer list.
4. Login with that customer in another browser, access the shop url : you can access.
5. Try to remove the customer from customer list : you see error below

Animated Gif/Screenshot

Context

Reported by various users in Can, Aus, Fr, etc.

Severity

~bug-s2: a non-critical feature is broken, no workaround~

See below for a more detailed discussion around this. Deleting a customer requires a new feature.

Possible Fix

Option 1 - delete a customer from your customer list regardless of whether they have placed orders in your shop or not (soft delete ? so the data remain but it becomes invisible ?)
Option 2 - create an 'inactive' customer - that is marked slightly differently on the list or hidden (but can be displayed if needed), that doesn't have access to the shop

[myriamboure]

ref discussion on community https://community.openfoodnetwork.org/t/private-shop-issues/866/2

[lin-d-hop]

The solution to #1457 should also cover this...

[myriamboure]

Hum... it will partly solve it @lin-d-hop , but not really. Let's say member A is member of the buying groups for one year, but stop being a member. The buying group who manages a private shop want to remove this user so that he doesn't have access anymore (or the hub should use a tag method but then the private shop feature is no point...) So for me if a hub want to remove a user with existing orders, he should be able to do it the existing orders should just stay as they were, a bit like when you remove a product, the existing orders with the product stay as they were... of course if existing order maybe a warning pop-in with "are you sure" is a good idea.

[jeronimo]

I think there was a serious reason to bind customer with order in a such stiff way. @lin-d-hop, maybe someone can help us identify why was that and if we can make it not that strong?

[myriamboure]

@lin-d-hop I think we need to keep the "bug" label as the private shop feature won't be usable without this... We already have hubs who have people in their customer list because they made tests, and then set up the shop as private but they don't want those people to access the shop... for me it's breaking the feature if you can't control who have access to your private shop.
In that sense I would use the same severity 2 tag as private shop bug 1 (connected to email confirm) as unfortunately I don't see any workaround, appart from asking a dev to hard delete the user from DB :-o
But of course happy to hear what you thought about it :-)

[sauloperez]

As a severity 2 bug it should be worked on next as soon as any dev is available. Is that what we want?

[myriamboure]

Yep, right @sauloperez ! There are probably various way to address that issue though (various candidates):

• do we just enable to delete the customer from the customer list (but the customer still exist in the system, no impact on previous orders, etc, just invisible from customer list view and no access to the shop for the given customer)
• or do we want the hub to be able to keep all the customer list but "tag" in some way those who can access the shop?
  I guess the first step resolution which will solve the bug and so enable to use the initial private shop feature is just to be able to delete the customer. And probably the feature can be improved later on. So my proposal would be to keep it simple for now, but just wanted to make sure the bug fix is acceptable as is.

[sauloperez]

So my proposal would be to keep it simple for now

:heart:

[HugsDaniel]

Someone working on this one @myriamboure ? I could take a look.

[myriamboure]

Would be great @HugsDaniel ! I don't think there is any connexion with "soft delete" here as it's just removing from view from hub manager and removing access, it's not really deleting anything.

I feel though that we need some consent from the community about the solution we propose as step one:
1- a hub manager can "remove" a customer from his customer list, so then the customer doesn't have access anymore to the shop. This solve the private bug issue. But it means also that the hub manager loose track of this ex-customer information, we need to be aware of that. It's a quick fix to the bug, but is not ideal in term of user customer management (like if the customer wants to shop again the manager wouldn't know he has shoped before). Are we ok though to start with that quick fix before we work on improving customer management as a whole? Of course old orders from the given customers will still be there.
2- else we need a speccing session to decide what we want...
@daniellemoorhead @sstead @lin-d-hop @sauloperez @enricostano @sigmundpetersen would be great to have your consent on solution one...

[enricostano]

I think it's too early to start working on this. Let's talk about it in the curation team kick-off and look at priorities.

[daniellemoorhead]

@myriamboure and @enricostano now that we've prioritised sev 1 and 2 bugs to be fixed shall we kick off determining a design/solution for this? Is this something you all can manage over there?

[enricostano]

@daniellemoorhead tomorrow with @myriamboure we'll go through s1 and s2 and will see if we need some help. In this specific case we will since the solution that is in the air (soft deletion) is part of a bigger sicussion that we still need to have.

[myriamboure]

After discussing with @enricostano this morning we agreed that this was not a bug but just the feature was not very well designed. So we created an icebox "hubs can decide who accesses their private shop" https://community.openfoodnetwork.org/t/hubs-can-decide-who-accesses-their-private-shop/1236 and this will go through the priorization process. Also started to list feature candidates, please feel free to add some on the thread if you have other ideas.

[myriamboure]

Given discussions here https://community.openfoodnetwork.org/t/hubs-can-block-shoppers-decide-who-accesses-their-private-shop/1236/12 we agreed that it is a bug and reopen it. We leave it s3 and as mentioned by Theresa in the discussion it blocked onboarding of 2 hubs. So let's fix it to make the private shop feature fully work.

[kirstenalarsen]

Why couldn't we just create an 'inactive' customer - that is marked slightly differently on the list or hidden (but can be displayed if needed), that doesn't have access to the shop?

[myriamboure]

Ok I updated the issue description as it was old and not precise enough to be dealt with. I added your option. We need to discuss with the dev who will be working on it which option is best to choose. Option 2 is obviously better UX if it is not more complex. Option 1 could solve the problem even if not ideal on UX side. But it would improve the situation so if it's lot easier the value matrix logic would go toward option 1.

[mkllnk]

Let me explain the problem here. It involves a bit of OFN history.

Introducing the customer

Customers were first introduced by @lin-d-hop in 87b1ab9a1acfaa2624868c475c202068bc3acd89 for reporting. An enterprise could give a customer a different name or identifier, the customer code. It would display in reports. The implementation was basically an association record between User and Enterprise.

Tagging customers and addresses

Later on Rob introduced tagging and tag rules. Since then we can add tags to customers and apply rules based on that. For example all customers with tag "trusted" would be able to see the cash payment method. The implementation added a second use to the customer record.

Later we added a third use and stored billing and shipping address on the customer record as well.

Customers access private shop fronts

When we came to implement private shop fronts we thought it's really easy. We just need to add an option to shops that they are private and then only customers can see them. At first it was just about seeing the shop, later we introduced the option that you can't buy without being a customer. We didn't think about removing customers. It wasn't part of the scenario.

Possible solutions

The customer record has many uses. We re-used it for private shop fronts, but it wasn't designed for private shop fronts. I think it would break other parts of the code if we tried to delete customer records now. The problem is not that these people aren't customers, the problem is that they are not allowed to see or buy in a certain shop.

I think we should add new records for shop permissions to the database so that you can choose which customers are allowed to see a shop and which are not. It could be as simple as adding a property private_shop_access to the customer model. This is basically the same as @kirstenalarsen suggested with an inactive flag. It would bloat all customer records though. The inactive flag wouldn't mean anything on a customer of a public shop enterprise. A better data model may be a new table private_shop_customers just containing the customer id and enterprise id pairs for those customers who are allowed.

I'm also wondering what we want in the future. Should permissions be on a pure user-enterprise basis? Or do we actually want it on a per order cycle basis so that one enterprise can have private and public shops? We could also use tag rules and give customers access based on their tags.

I'm calling it a day and won't work on this before Tuesday. Maybe somebody wants to go ahead and demonstrate a solution with the inactive flag which is the easiest for now. We just need a new input element on the customers page and change the database model slightly.

[myriamboure]

Interesting. I had some sames thoughs when working on pricing tables. Let me share some insights that can help in that discussion.

Customer groups
I realized when we started to talk about "customer categories", that tags was a way to organize customer categories and give different "options" to those categories : different shipping methods, payment methods, OC, and next, "offers/prices". the "tag" UX is pretty confusing / complex for users, but the logic behind it feels right.
Instead of having the tags in the shop setup, they could be in the future, with another UX on "customer groups", on the customer section of the app.

Adding some customer in a specific customer group (= giving him/her a specific tag) would give him different kind of rights/options. One of them is already "see the OC or not".

So actually, there is already a workaround to this bug which is to tag the customer "unauthorized" and set up a tag rule: default = OC visible (but as private only customers will see, else welcome private shop message), and if user has tag = "unauthorized", then OC becomes invisible. Of course the problem with this UX is that hub manager needs to think to add the tag to the OC...
Also, another UX issue is that the message saying "the shop is private, contact X" won't be displayed. So UX for unauthorized shoppers is pretty poor.
But @kirstenalarsen only the users who are in customer list and unauthorized won't see the welcome private shop message, so it seems an acceptable temporary workaround if that prevents onboarding new users.

Shot term solution
So I agree with you @mkllnk that I think we will review all that at some point and that it should be OC based and not enterprise based, but I also think that we have other priorities at the moment, so if this "inactive flag" solution is easy (like 1 day of work) I would just do it. Fix the bug now, improve / review the feature later.

[mkllnk]

I would imagine a day of dev work to create a pull request. It may take a few more hours to reply to pull requests etc. I would be more comfortable saying it's 1-3 days, because it's a database change, logic change and UX change, affecting several parts of the system.

[daniellemoorhead]

I agree with @myriamboure, let's do the fix as it's a 1-3 day piece and should be resolved as soon as possible.

@mkllnk perhaps you can pick it up and run with it, given your understanding?

[sauloperez]

I went through the comments and I'm giving it a try so that we work on it before you are available @mkllnk.

[sauloperez]

After the insightful input from @mkllnk and @lin-d-hop I better understand now what's the situation. Fundamentally, there are few features tight together in the Customer model. Granting access to a private shop, the list of customers a hub manager wants to keep track of (a CRM-like idea of customer) and tagging: giving access depending on criteria defined by a manager. Is that it @mkllnk ?

Now, however, there's no way I can do one without the others. If I want to add a person to the list (to keep her in the loop of my hub) and I happen to have a private shop, I'm granting her access to it. If I made a mistake I can't delete the customer and I understand that's annoying. Also, if I remove the customer the underlying user we'll skip any tagging rules and end up buying the variant I don't want her to. You see? the Customer model serves multiple purposes at the same time.

So these various use cases need to be decoupled so that I can do one without the others. Then, my question is. How should we proceed? This issue's expectation is explicitly related to revoking access to private shops but the issue's title relates to enabling deletes. First, we need to separate these two issues and start with one.

I started exploring the idea of soft-deleting customers in https://github.com/openfoodfoundation/openfoodnetwork/pull/3457 (soft-delete = flag it as deleted and filter out deleted rows everywhere, which avoids us from losing data), but that doesn't address this coupling issue. We still experience the problems I described above. Also, it's not clear to me if someone is using the customers' list as a CRM, if that was the case I'd go with #3457 which is nearly finished. Still, with these problems but it would solve the private access revocation.

On the other hand, we can implement explicit permissions to grant access to private shops but that requires a bit of UI and deeper thinking. IMO it should be then addressed as a feature and not a bug, although removing customers feels on the edge as well. We could perhaps start with the delete and consider explicit permissions meanwhile if we think it's a priority.

In any case, let's not rush and get a Frankenstein out the door. Let's reach consensus first. I'll close #3457 meanwhile as it's blocked and the Spree upgrade should be our focus.

[daniellemoorhead]

@kirstenalarsen @lin-d-hop @RachL (on behalf of Myriam) perhaps at this point in time it's good to get the product owners together to determine the best approach for this - over to you all 😄

[daniellemoorhead]

PS. I've moved this back to the top of the bugs backlog as it isn't dev ready and needs clarification on how it should be moved forward.

[mkllnk]

Great summary @sauloperez. Solving this bug requires a new feature and probably a UX change to allow and deny customers.

[RachL]

@mkllnk @sauloperez would you say this is something to tackle after the spree upgrade? If yes I would suggest adding the blocked label and after spree upgrade, putting this inception among the priorities (we have a couple of s1 bugs that are also waiting the upgrade).

What do you think @kirstenalarsen @lin-d-hop ?

[sauloperez]

I don't think this is necessarily dependant on the upgrade other than in terms of available resources while we work on v2. A proper inception will take time anyway.

[daniellemoorhead]

I would suggest adding the blocked label and after spree upgrade, putting this inception among the priorities (we have a couple of s1 bugs that are also waiting the upgrade).

@RachL therefore should this become something on the wishlist rather than continuing to be dealt with as a bug?

[RachL]

@daniellemoorhead it feels to me yes. However there have been some intense discussions around this already, so I'm feeling a bit late to the party to reopen this. So in the meantime I would like to understand if everyone agrees this can wait til spree upgrade is over, that's one thing I'm not sure I understood from the discussion I have been reading.

[mkllnk]

I agree that this is not a bug but a feature. ;-) We have a workaround with tags, right? I'll take the label off and move it.

[myriamboure]

I'm sorry but I don't understand the conclusion. If it's not a bug it's not only removing the label, but having an appropriate wishlist to treat it. That's what I had previously done, I had closed that issue and opened a wishlist. But with discussions people argued and we finally said it was a bug so reopened it. No problem to go again the other side, but then if we say it's not a bug we need to make sure there is a wishlist and close that issue. My concern is that was blocking both Theresa en Kirsten with users, so I think we need to check with them if the tag workaround is ok and usable for their users. But we can't have an issue with no label hanging here...

[sauloperez]

Would you mind moving it back to a wishlist with the summary I shared in https://github.com/openfoodfoundation/openfoodnetwork/issues/1494#issuecomment-463587103? It gives us the whole picture to assess it properly. I would then close this issue.

[sigmundpetersen]

Summary of investigation moved to Discourse 1236