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

Bring spree_backend search controller to OFN #4511

Merged

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Nov 27, 2019

What? Why?

Related to #4050

We dont bring much from spree here, we basically convert the existing search controller decorator into a controller.

The rubocop manual todo was updated for the abc size warning.

What should we test?

This is well covered with auto tests but we can briefly verify:

Known users

  • enterprise owner selection when creating an enterprise, should only show for super admin so the list of users should be total
  • enterprise owner selection and adding new manager when editing an enterprise
    • these two lists should only show for owner or super admin, if owner, the list of users that should show is the list of managers of enterprises where the current user is a manager.
  • In creating and editing a group: should only show for super admin and show the list of all users

Customer search

  • Only used while editing an order in the cart state, go to customer details and search for a customer, the list of customers should be customers of the order's distributor whose email or name match the text input.

Release notes

Changelog Category: Changed
Simplified code related to searching users.

@luisramos0 luisramos0 self-assigned this Nov 27, 2019
@luisramos0 luisramos0 force-pushed the backend_ctrl_search branch 3 times, most recently from 7ec1262 to f4a22d6 Compare December 14, 2019 10:53
@luisramos0 luisramos0 changed the title WIP Bring spree_backend search controller to OFN Bring spree_backend search controller to OFN Dec 14, 2019
@luisramos0 luisramos0 removed the pr-wip label Dec 14, 2019
@sigmundpetersen
Copy link
Contributor

Needs a rebase

@luisramos0
Copy link
Contributor Author

👍 done

@filipefurtad0 filipefurtad0 self-assigned this Feb 4, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Feb 4, 2020
@luisramos0
Copy link
Contributor Author

rebased to resolve conflicts again.

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Feb 4, 2020
@filipefurtad0
Copy link
Contributor

Hey @luisramos0,
A quick notice to let you know thar this PR needs atention again, new conflicts arose.
Thank you!

@luisramos0
Copy link
Contributor Author

Hey Filipe, where do you see the conflicts? Looks good to me.

The broken build was a flaky spec, I am rerunning it. Anyway, this is ready for testing.

@filipefurtad0
Copy link
Contributor

Humm. I could swear there was a error before - but appears to be gone, now. Great! Proceeding with staging/testing.

@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Feb 4, 2020
@filipefurtad0
Copy link
Contributor

Hi @luisramos0 ,

Sorry it took so long, this one was a bit tricky. I think it is good to go, but found issues which may be relevant.

Considering your testing notes, I found nothing considering the your section Known Users. All good here.

However, concerning Customer Search, users which added items to a cart, may - under certain circumstances (see below) - be found under the Customers menu, accessible as Superadmin (https://staging.openfoodnetwork.org.uk/admin/customers).

For this to happen, the order has to be edited first:

  • one needs to search for the respective Order, using the email of the user/customer, and unchecking "ONLY SHOW COMPLETE ORDERS". So, the Order should be in the Cart state:

  • click Edit

  • click on Customer Details. Nothing has to be changed, just accessing these menus should do it.

  • After this, clicking on the Customers menu, the User should appear as Customer, although no purchase was made (everything still in the Cart state)

Also, in some cases, an Order in cart appears with the Address Status. I would further need to investigate this, as it does not always seem to happen.

Appearing as a Customer - with no purchase made - might raise the discussion, on whether we should keep addresses or data from users, in the Cart state.

I think the PR is ready to go, but these are issues that maybe should be followed up.

Thanks again @luisramos0 ! Was this the last piece of Spree code? :tada Thank you too @RachL, for the help and discussion on this.

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Feb 5, 2020
@luisramos0 luisramos0 merged commit 9313a57 into openfoodfoundation:master Feb 5, 2020
@luisramos0 luisramos0 deleted the backend_ctrl_search branch February 5, 2020 15:54
@luisramos0
Copy link
Contributor Author

cool, thanks Filipe.
As in a previous PR, from this PR perspective what's really important is to know if these behaviours are existing or introduced in this PR. I'd test these things by deploying master to the same staging server or test in prod if possible.
It looks like existing behaviour, so I am merging. But we should verify if they really are existing issues or not.

@filipefurtad0
Copy link
Contributor

Yes, @luisramos0,
Thanks. I should have mentioned that the behaviour is not PR dependent, i.e., this was checked in FR, UK and KA, for newly added items, to carts. There was slightly different responses, but the described procedure applies to all.

So, I fully agree with you, this deserves extensive testing, and tracking out these small differences, but I they don't seem to be related to this PR.

@luisramos0 luisramos0 mentioned this pull request Feb 6, 2020
18 tasks
@luisramos0
Copy link
Contributor Author

Some orders will be in the address state if something went wrong with the address on checkout. It's not important because if the user goes to the checkout page again (just loading the checkout page as that user is enough), the state will be moved to cart again.

I dont think the other point is an issue, from what I understand you are saying that users who have an open cart order with the distributor will also show up on their customers list. I think that could be useful.

From what I understand these are the two questions you raised above. I think we can leave it as is.

@mkllnk
Copy link
Member

mkllnk commented Feb 11, 2020

users who have an open cart order with the distributor will also show up on their customers list.

Uh, that's new. How does that work with customer-only shops? Do they need to add a product (see the shop) or does everybody automatically become a customer and the customer-only feature is completely defunct?

@luisramos0
Copy link
Contributor Author

good point Maikel.
I think it's not really a normal cart, you know that when a checkout fails the order is set to cart again, maybe the order as gone through the workflow, the customer was created and then reset to cart...
I didnt check these things, just thinking..

@filipefurtad0
Copy link
Contributor

I will further check these issues.
I am not entirely sure this is new, or was there before, and ran unnoticed. Further feedback soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants