Hide users with no completed orders from a hub's customers list #10704

abdellani · 2023-04-15T04:56:59Z

What? Why?

Closes Hide users with no completed orders from a hub's customers list #10624

I update the three endpoints that return customers list to only return:

customers created manually
or customers that have at least one completed order

Lists should not return customer that have started (and not completed) a checkout with split checkout.

What should we test?

check the customers list
check customers list endpoints (v0 & v1)

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

spec/models/customer_spec.rb

abdellani · 2023-05-16T08:12:37Z

I used the scope Order::complete which includes:

completed orders
resumed orders
completed at least one order then canceled it, I'm not sure if we need those on the list (check comment)

For some reason, there is one test failing. I don't know why.

jibees · 2023-05-16T13:23:11Z

For some reason, there is one test failing. I don't know why.

Following spec fails:

openfoodnetwork/spec/controllers/admin/customers_controller_spec.rb

Lines 47 to 55 in bab45fa

    
           it 'calls CustomersWithBalance' do 
        
             customers_with_balance = instance_double(CustomersWithBalance) 
        
             allow(CustomersWithBalance) 
        
               .to receive(:new).with(customers) { customers_with_balance } 
        
             expect(customers_with_balance).to receive(:query) { Customer.none } 
        
             get :index, params: params 
        
           end

This spec is testing app/controllers/admin/customers_controller.rb and calls

openfoodnetwork/app/controllers/admin/customers_controller.rb

Lines 69 to 81 in bab45fa

    
           def collection 
        
             if json_request? && params[:enterprise_id].present? 
        
               CustomersWithBalance.new(customers).query. 
        
                 includes( 
        
                   :enterprise, 
        
                   { bill_address: [:state, :country] }, 
        
                   { ship_address: [:state, :country] }, 
        
                   user: :credit_cards 
        
                 ) 
        
             else 
        
               Customer.where('1=0') 
        
             end 
        
           end

(via app/controllers/admin/resource_controller.rb)

collection() is calling is customers() which has been changed in your PR.

Error is saying:

<CustomersWithBalance (class)> received :new with unexpected arguments

so it appears that customers is not well defined.
customers is defined like this:

openfoodnetwork/spec/controllers/admin/customers_controller_spec.rb

Line 34 in bab45fa

    
           let(:customers){ Customer.managed_by(user).where(enterprise_id: enterprise.id) }

I suppose it should be defined like this:

let(:customers){ Customer.visible.managed_by(user).where(enterprise_id: enterprise.id) }

and then the spec pass.

What do you think?

mkllnk

This mainly good. We just need to enable the API v1 to create visible customers as well.

I personally would have preferred an active field because that could be re-used for other purposes later but I'm okay with your given the current requirements.

mkllnk · 2023-05-17T05:48:01Z

db/migrate/20230516072511_add_created_manually_flag_to_customer.rb

@@ -0,0 +1,5 @@
+class AddCreatedManuallyFlagToCustomer < ActiveRecord::Migration[7.0]
+  def change
+    add_column :customers, :created_manually, :boolean, default: false


Customers that were created manually in the past will disappear now.

Yes, the customers that were created manually and don't have at least one completed/resumed order will disappear.

There's no way to distinguish between the two.

I think a subset of them are the customers who don't have any orders associated with them (regardless of the state of the order). Maybe we can add a migration to update their flag?

if we add that migration, we'll only lose the customers created manually, have at least incomplete order, and don't have any completed order (the three conditions must be verified).

what do you think?

That's a good idea. We can look for customers without any orders and it's safe to assume that they were added manually. We cant distinguish the others... hm, what about the created_at column of order and customer? If a customer record was created more than a minute before the order record, it must have been created manually.

Uh, another thing is tags. If enterprises assigned tags to customers then they probably want to continue to see them. That makes me wonder what happens if an enterprise tries to create a customer which already exists but is invisible. It probably fails. And there's no way of tagging such a customer. We need a way to "activate" customers or we just switch them to manually created if we detect this.

what happens if an enterprise tries to create a customer which already exists but is invisible. It probably fails. And there's no way of tagging such a customer.

^ This seems really important. We probably should provide a filter option so that users can choose to show/hide based on this field (I think this would apply to the admin interface and API).

Even then, it may not be clear when you get the error "customer already exists" but you can't see them. So we might need to add a hint in the "customer already exists" message (assuming there is one).

app/models/customer.rb

spec/models/customer_spec.rb

mkllnk · 2023-05-17T05:58:58Z

app/controllers/api/v1/customers_controller.rb

      def visible_customers
-        Customer.managed_by(current_api_user)
+        Customer.visible.managed_by(current_api_user)
      end

      def customer_params


I think we should allow this field to be set in the API. One use case of the API is to import customer records which needs this field set. So maybe it should be on by default.

It's not on by default but that's probably okay.

Sorry, I misunderstood your comment.
I'll set that flag by default to the ~~imported records~~ new record.

abdellani · 2023-05-17T08:17:08Z

I suppose it should be defined like this:
let(:customers){ Customer.visible.managed_by(user).where(enterprise_id: enterprise.id) }
and then the spec pass.

What do you think?

Yes, you're right @jibees

So, It's comparing the queries and not the results of the queries.

I was confused because in the logs I could not see any differences between the given and the expected values.

Thank you 🙏

mkllnk

Good improvements. It would be easier to review if you only added commits after it has been reviewed. And I think I found another scenario that needs addressing before we proceed.

mkllnk · 2023-05-19T05:19:58Z

db/migrate/20230516072511_add_created_manually_flag_to_customer.rb

@@ -0,0 +1,5 @@
+class AddCreatedManuallyFlagToCustomer < ActiveRecord::Migration[7.0]
+  def change
+    add_column :customers, :created_manually, :boolean, default: false


That's a good idea. We can look for customers without any orders and it's safe to assume that they were added manually. We cant distinguish the others... hm, what about the created_at column of order and customer? If a customer record was created more than a minute before the order record, it must have been created manually.

Uh, another thing is tags. If enterprises assigned tags to customers then they probably want to continue to see them. That makes me wonder what happens if an enterprise tries to create a customer which already exists but is invisible. It probably fails. And there's no way of tagging such a customer. We need a way to "activate" customers or we just switch them to manually created if we detect this.

mkllnk · 2023-05-19T05:20:48Z

app/controllers/api/v1/customers_controller.rb

      def visible_customers
-        Customer.managed_by(current_api_user)
+        Customer.visible.managed_by(current_api_user)
      end

      def customer_params


It's not on by default but that's probably okay.

mkllnk

Good effort. I think that we definitely need an index on the new column and the migration may blow up on large datasets in production.

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb

db/migrate/20230516072511_add_created_manually_flag_to_customer.rb

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb

dacook

This looks good, but I'm hesitant to approve because it sounds like a user could be blocked from finding/updating customers. Is that the case?

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb

dacook · 2023-05-26T04:06:01Z

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb

+      .or(Customer.where.not(id: customers_with_at_least_one_order))
+      .or(
+        Customer.where(id: customers_created_more_than_1min_before_orders)
+        .where.not(id: customers_created_no_more_than_1min_before_orders)


This seems redundant to me, it's the same as the previous line isn't it?

That is, they are mutually exclusive. If

"#{creation_time_difference_in_seconds_sql} >= 60"

then it cannot be

"#{creation_time_difference_in_seconds_sql} < 60"

dacook · 2023-05-26T04:09:58Z

db/migrate/20230516072511_add_created_manually_flag_to_customer.rb

@@ -0,0 +1,5 @@
+class AddCreatedManuallyFlagToCustomer < ActiveRecord::Migration[7.0]
+  def change
+    add_column :customers, :created_manually, :boolean, default: false


what happens if an enterprise tries to create a customer which already exists but is invisible. It probably fails. And there's no way of tagging such a customer.

^ This seems really important. We probably should provide a filter option so that users can choose to show/hide based on this field (I think this would apply to the admin interface and API).

Even then, it may not be clear when you get the error "customer already exists" but you can't see them. So we might need to add a hint in the "customer already exists" message (assuming there is one).

filipefurtad0 · 2023-05-29T10:13:57Z

After discussion with @abdellani we've added the feedback needed label - more context here.

mkllnk

I'm happy with my own changes. 😉

dacook

Great team effort!

As Lynne confirms, we won't support accessing of hidden users in this PR.

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb

The form only submits enterprise id and email address. We don't need to assign any other attributes.

The API endpoint merges the created_manually flag in the params already. No need to write it separately.

It should only be true or false. This was flagged by Rubocop. I also added another Rubocop suggestion and combined two migrations because they are related.

mkllnk

I removed the error handling again, sorry @rioug. And I cleaned up a little. Would be good to get another perspective here.

dacook

Great, I'm happy with this, knowing the spec now covers the situation.

But I can see room for improvement. It looks like a simple change so I'll try it out..

app/models/customer.rb

dacook

All good on my end now. Either @mkllnk or @abdellani, if you're happy to approve, I think we are ok for testing?

mkllnk

Great change!

Four people workin on one PR...

db/schema.rb

dacook · 2023-06-15T06:28:12Z

Strange to have a controller spec randomly fail. Works locally for me. Retrying...

rspec ./spec/controllers/admin/bulk_line_items_controller_spec.rb:55

filipefurtad0 · 2023-06-15T09:48:47Z

Strange to have a controller spec randomly fail. Works locally for me. Retrying...

I've reproduced this locally both on master and after pulling this branch, opened issue here. The branch does not make it worse, so we're good for testing it.

filipefurtad0 · 2023-06-29T22:21:04Z

Hey @abdellani ,

Thanks so much for this effort 🙏

Before staging, I've created three customers:

one of them, created during an incomplete checkout (the scenario which sparked the issue) - the tapascustomer
another created, manually (on the /customers page) - the tapascustomer_created
and yet another one, created by completing a checkout, in the shop-front - the tapascustomer_completed

Before staging this PR, the /customers page looks like this, for the respective hub:

After this PR:

Customer 1) is not appearing, but also, customer 2) (created manually) is missing; only customer 3) appears:

I've looked at your tests and they seemed good to me, so the next step, was to look at the DB:

openfoodnetwork=> select * from customers where email like '%tapas%' and enterprise_id = 2483;

  id  |                      email                      | enterprise_id | code | user_id |         created_at         |         updated_at         | bill_address_id | ship_address_id | name | allow_charges | terms_and_conditions_accepted_at | first_name | last_name | created_manually 
------+-------------------------------------------------+---------------+------+---------+----------------------------+----------------------------+-----------------+-----------------+------+---------------+----------------------------------+------------+-----------+------------------
 7101 | filipefurtado+tapascustomer@gmail.com           |          2483 |      |    7914 | 2023-06-29 21:51:00.437896 | 2023-06-29 21:51:00.437896 |          348292 |          348293 |      | f             |                                  | Tapas      | Customer  | f
 7102 | filipefurtado+tapascustomer_created@gmail.com   |          2483 |      |         | 2023-06-29 21:52:04.074909 | 2023-06-29 21:52:04.074909 |                 |                 |      | f             |                                  |            |           | f
 7103 | filipefurtado+tapascustomer_completed@gmail.com |          2483 |      |    8026 | 2023-06-29 21:54:46.971582 | 2023-06-29 21:55:03.240342 |          348294 |          348297 |      | f             | 2023-06-29 21:55:03.240342       | first      | last      | f
(3 rows)

It looks like the created_manually is not working as expected... as the value is f for all three customer entries.

Does this test help you move forward? Is there a particular scenario which you feel needs to be tested?

mkllnk · 2023-06-29T23:38:03Z

The migration looks good to me. Maybe it was already applied before it didn't run again?

I just reverted and repeated the migration:

./bin/rails db:migrate:down VERSION=20230525081252
./bin/rails db:migrate

Then checked the database again. Thank you for posting your query:

openfoodnetwork=> select id, email, created_manually from customers where email like '%tapas%' and enterprise_id = 2483;
  id  |                      email                      | created_manually 
------+-------------------------------------------------+------------------
 7101 | filipefurtado+tapascustomer@gmail.com           | f
 7103 | filipefurtado+tapascustomer_completed@gmail.com | f
 7102 | filipefurtado+tapascustomer_created@gmail.com   | t
(3 rows)

It seems to work. I'll let you finish the testing.

filipefurtad0 · 2023-06-30T09:56:18Z

Ahh interesting, thanks @mkllnk ,

I did not think the migration hadn't worked, as the created_manually attribute was there 👀
But indeed, after you've re-ran the migration it does work!

We see that, in the /customers page:

the manually created customer is appearing (created_manually = true in the DB) 🟢
the customer with completed orders is appearing (created_manually = false in the DB) 🟢
the customer neither created manually nor having any completed orders does not appear (created_manually = false in the DB) 🟢

I was wondering what happens if we cancel an order: would then the customer disappear from the customers list?

I've tested this and the answer is no - for a customer with one order only:

cancelling the order -> the customer remains visible
resuming the order -> the customer remains visible

I think this is great and makes sense, as the user already has completed orders in the past.

This looks great!
Merging. Thanks again for your help @mkllnk 🙏

abdellani self-assigned this Apr 15, 2023

abdellani force-pushed the hide-customers-with-no-completed-orders branch from ae5c705 to bab45fa Compare April 18, 2023 05:47

abdellani commented Apr 18, 2023

View reviewed changes

spec/models/customer_spec.rb Outdated Show resolved Hide resolved

abdellani force-pushed the hide-customers-with-no-completed-orders branch 2 times, most recently from 3e2bcbe to de0175c Compare May 16, 2023 08:04

abdellani marked this pull request as ready for review May 16, 2023 08:04

abdellani force-pushed the hide-customers-with-no-completed-orders branch 2 times, most recently from a789882 to 77aee44 Compare May 16, 2023 09:07

sigmundpetersen requested a review from jibees May 16, 2023 11:13

mkllnk requested changes May 17, 2023

View reviewed changes

abdellani force-pushed the hide-customers-with-no-completed-orders branch 2 times, most recently from 691e60c to 5adb6cb Compare May 17, 2023 08:19

sigmundpetersen requested a review from mkllnk May 17, 2023 16:52

mkllnk requested changes May 19, 2023

View reviewed changes

abdellani force-pushed the hide-customers-with-no-completed-orders branch from 5adb6cb to 875e996 Compare May 25, 2023 08:12

abdellani requested a review from mkllnk May 25, 2023 10:22

dacook self-requested a review May 26, 2023 03:38

mkllnk requested changes May 26, 2023

View reviewed changes

dacook reviewed May 26, 2023

View reviewed changes

abdellani added the feedback-needed label May 29, 2023

abdellani force-pushed the hide-customers-with-no-completed-orders branch 2 times, most recently from 6b04272 to ed2deb3 Compare May 30, 2023 19:22

abdellani requested review from mkllnk and dacook May 30, 2023 19:47

mkllnk approved these changes May 31, 2023

View reviewed changes

dacook approved these changes May 31, 2023

View reviewed changes

db/migrate/20230525081252_update_created_manually_flag_on_customers.rb Show resolved Hide resolved

mkllnk added 6 commits June 15, 2023 12:02

Place class methods above instance methods

22a6861

Remove useless code

ebebcf7

Change spec to "unhide" customers on create

9bd6615

Remove unnecessary writes on customer creation

a655d3d

The form only submits enterprise id and email address. We don't need to assign any other attributes.

Simplify customer code

75cce8b

The API endpoint merges the created_manually flag in the params already. No need to write it separately.

Remove NULL possibility from customer flag

a46917a

It should only be true or false. This was flagged by Rubocop. I also added another Rubocop suggestion and combined two migrations because they are related.

mkllnk force-pushed the hide-customers-with-no-completed-orders branch from e6a27c8 to a46917a Compare June 15, 2023 02:02

mkllnk approved these changes Jun 15, 2023

View reviewed changes

dacook requested changes Jun 15, 2023

View reviewed changes

app/models/customer.rb Outdated Show resolved Hide resolved

app/models/customer.rb Outdated Show resolved Hide resolved

Use builtin Rails method

d195882

dacook approved these changes Jun 15, 2023

View reviewed changes

mkllnk approved these changes Jun 15, 2023

View reviewed changes

db/schema.rb Outdated Show resolved Hide resolved

dacook force-pushed the hide-customers-with-no-completed-orders branch from 1cee987 to d195882 Compare June 15, 2023 06:06

jibees mentioned this pull request Jun 16, 2023

Add workflow for deploying to staging #11031

Merged

drummer83 self-assigned this Jun 24, 2023

drummer83 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 24, 2023

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jun 29, 2023

filipefurtad0 self-assigned this Jun 29, 2023

filipefurtad0 added the pr-staged-au staging.openfoodnetwork.org.au label Jun 29, 2023

filipefurtad0 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Jun 29, 2023

mkllnk removed the feedback-needed label Jun 29, 2023

filipefurtad0 merged commit b175793 into openfoodfoundation:master Jun 30, 2023
57 of 59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide users with no completed orders from a hub's customers list #10704

Hide users with no completed orders from a hub's customers list #10704

abdellani commented Apr 15, 2023 •

edited by mkllnk

abdellani commented May 16, 2023 •

edited

jibees commented May 16, 2023 •

edited

mkllnk left a comment

mkllnk May 17, 2023

abdellani May 17, 2023

mkllnk May 19, 2023

dacook May 26, 2023

mkllnk May 17, 2023

abdellani May 17, 2023

mkllnk May 19, 2023

abdellani Jun 7, 2023 •

edited

abdellani commented May 17, 2023

mkllnk left a comment

mkllnk May 19, 2023

mkllnk May 19, 2023

mkllnk left a comment

dacook left a comment

dacook May 26, 2023

dacook May 26, 2023

filipefurtad0 commented May 29, 2023

mkllnk left a comment

dacook left a comment

mkllnk left a comment

dacook left a comment

dacook left a comment

mkllnk left a comment

dacook commented Jun 15, 2023

filipefurtad0 commented Jun 15, 2023

filipefurtad0 commented Jun 29, 2023

mkllnk commented Jun 29, 2023

filipefurtad0 commented Jun 30, 2023

Hide users with no completed orders from a hub's customers list #10704

Hide users with no completed orders from a hub's customers list #10704

Conversation

abdellani commented Apr 15, 2023 • edited by mkllnk

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

abdellani commented May 16, 2023 • edited

jibees commented May 16, 2023 • edited

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdellani Jun 7, 2023 • edited

Choose a reason for hiding this comment

abdellani commented May 17, 2023

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented May 29, 2023

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook commented Jun 15, 2023

filipefurtad0 commented Jun 15, 2023

filipefurtad0 commented Jun 29, 2023

mkllnk commented Jun 29, 2023

filipefurtad0 commented Jun 30, 2023

abdellani commented Apr 15, 2023 •

edited by mkllnk

abdellani commented May 16, 2023 •

edited

jibees commented May 16, 2023 •

edited

abdellani Jun 7, 2023 •

edited