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

Update counts in OC edit page to exclude hidden overrides #4806

Open
Matt-Yorkley opened this issue Feb 19, 2020 · 10 comments
Open

Update counts in OC edit page to exclude hidden overrides #4806

Matt-Yorkley opened this issue Feb 19, 2020 · 10 comments
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround.

Comments

@Matt-Yorkley
Copy link
Contributor

Description

Part of #4355

Counts shown in OC edit page include hidden overrides that cannot be added to the OC, resulting in confusing feedback.

See additional screenshots and notes here: #4355 (comment) and here: #4355 (comment)

Expected Behavior

Variant counts shown in the OC edit page should make sense.

Actual Behaviour

The numbers are misleading, eg: "4 of 2 Variants loaded".

Steps to Reproduce

  1. Add some variants to inventory, and hide them
  2. View the variant counts for a related OC

Workaround

None.

Severity

bug-s4: it's annoying, but you can use it

@luisramos0
Copy link
Contributor

Hmm. trying to figure out an easy way out of this one...

I thought I could make hidden variants visible but not editable. They would show up but be disabled and with a new tooltip:
Screenshot from 2020-05-26 15-35-36
The counts would be correct because they would be included now.
Thoughts?

@Matt-Yorkley the code would be something like this: #5508

@Matt-Yorkley
Copy link
Contributor Author

LGTM 👍

Does it work OK with the select all button?

@luisramos0
Copy link
Contributor

this needs product feedback, probably better to double check that @RachL @lin-d-hop and @kirstenalarsen dont see any issues with this decision: merge the two cases 1. visible only variant (user has no permissions to add to exchange) and 2. hidden variant (variant is marked as hidden in inventory)

@RachL
Copy link
Contributor

RachL commented Jun 15, 2020

@luisramos0 just too be sure: the idea is that instead of not counting visible only variants and hidden variants, we would continue to count them but display them to the user as unselected and they would be able to select them but would have a tooltip to understand why. Is that correct?

I think I like the logic because it saves time. User don't waste time in searching why there are missing variants. But I have 2 questions:

  • what is the scope of the visible only variant? What kind of permissions are in place for this case to happen?
  • how does this rule apply with the select all? With a very large catalog, it would be difficult to find those that are unselected. Can we put them on top of the list?

@luisramos0
Copy link
Contributor

luisramos0 commented Jun 15, 2020

the idea is that instead of not counting visible only variants and hidden variants, we would continue to count them but display them to the user as unselected and they would NOT be able to select them but would have a tooltip to understand why. Is that correct?

YES, with correction above (NOT). The checkbox is disabled in the screenshot.

what is the scope of the visible only variant? What kind of permissions are in place for this case to happen?

It's complicated... this is the description in the code:
"my incoming producers' variants that are already in an outgoing exchange of this hub"
I think this is the case where the variant is already added but the user looking at the OC doesnt have add to OC permission.

how does this rule apply with the select all? With a very large catalog, it would be difficult to find those that are unselected. Can we put them on top of the list?

Select all would not select these, variants hidden in inventory and not-editable variants are not selectable.
Changing the order based on this will not be an easy thing. It would not make sense to move the "not-editable variants" to the top, right?
I am not sure how to solve that problem for large catalogs. Do we have to solve that problem?

@tschumilas
Copy link

I echo the concern about large catalogues. In OFN Canada now users use the inventory tool either because (1) they made a mistake and thought the had to put their products there, or (2) they have a large catalogue and its easier to manage it in inventory - mostly because you can hide things from view. so we need to think about large product lists - I think yes, we need to solve it.

@luisramos0
Copy link
Contributor

thanks @tschumilas I am not sure I was clear about the definition of the problem. The problem is not generic, like large catalogs. The problem is about hidden variants in large catalogs.

I think in practice we are talking about a user who is looking for a product in the shopfront with 100s of products. They cant find it there so they go to the OC edit page with those 100s of products to see what's going on. The natural process will be for the user to search for that product name in the exchange using the browser search?
Before my solution here: the product was not listed because it was hidden.
After this solution here: the product is there with disabled checkbox, the user can find the product and read the tooltip that explains why it is not available (hidden).
Does this make sense?

@kirstenalarsen kirstenalarsen added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Jun 23, 2020
@luisramos0
Copy link
Contributor

In Delivery train meeting we agreed this solution is not good enough because hiding variants is a feature user will want and with this we would be showing all the hidden variants.

A new option was suggested: if there are hidden variants in the page, we keep the numbers broken but show a message like "there are hidden variants in your inventory".

Or we can go back to "just fix the numbers" :-)

@luisramos0
Copy link
Contributor

luisramos0 commented Jun 23, 2020

For future reference, the problem is here:
https://github.com/openfoodfoundation/openfoodnetwork/blame/973ea2ea2d571e0cf498e3036f14edcb65b021db/app/assets/javascripts/admin/order_cycles/controllers/order_cycle_exchanges_controller.js.coffee#L59

We get the list of eligible products in the exchange from the server and we count all variants of these products. To fix the numbers we need to either make the server remove the hidden variants from the payload or make a new process that removes the number of hidden variants from the total number.

I investigated this a little bit now, maybe we could make the product serializer filter out hidden variants. I created a PR with that: #5656
I dont think this is correct though, this is not showing variants that are hidden in the coordinators inventory but this is for the case when the coordinator inventory setting is not selected in the OC. I think it should not show variants that are hidden in the exchange enterprise's inventory, not the coordinator.
Technically, I think the solution is what's in #5656 but filtering on the exchange.receiver, not order_cycle.coordinator. But I am not sure how we can easily get the exchange data into that serializer...

I am leaving this issue for now.

@luisramos0
Copy link
Contributor

I am closing the PR for now as well, here's the code:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround.
Projects
Status: All the things 💤
Development

No branches or pull requests

5 participants