Allow producer to edit their products on hubs' orders #13113

chahmedejaz · 2025-02-01T23:54:01Z

⚠️ Please use clockify code #12476 Flower Farms

What? Why?

Closes Allow producer to edit their products on hubs' orders #13031
This PR incorporates the ability for producers to edit their products on hubs' orders as per the features mentioned in the issue

What should we test?

As mention in the issue
Along with this, please test the whole existing order module

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

chahmedejaz · 2025-02-16T23:25:09Z

app/models/spree/ability.rb

+
+    # Users can manage line items in orders if they have producer enterprise and
+    # any of order distributors allow them to edit their orders.
+    def can_manage_line_items_in_orders?(user)


Adding the same user method names here for the sake of writing ability specs to better show the assigned abilities

rioug

Hi @chahmedejaz It's a good start, but I would like to see more unit test. It's a bit hard to follow if all changes are covered by some specs as you added them at the end. In general it's better to commit the specs whit the actual changes, at least the unit test.
I am wondering if we should also add a some system spec, at least for the order edit page.

app/services/permissions/order.rb

app/models/enterprise.rb

app/models/spree/ability.rb

app/models/spree/user.rb

rioug · 2025-02-16T23:39:14Z

app/services/permissions/order.rb

+      if @user.can_manage_line_items_in_orders_only?
+        Spree::LineItem.editable_by_producers(
+          @permissions.managed_enterprises.select("enterprises.id")
+        )
+      else
+        Spree::LineItem.where(order_id: editable_orders.select(:id))
+      end


lib/open_food_network/scope_variants_for_search.rb

app/views/spree/admin/orders/_table_row.html.haml

rioug · 2025-02-16T23:56:12Z

spec/services/permissions/order_spec.rb

@@ -4,7 +4,7 @@

 module Permissions
  RSpec.describe Order do
-    let(:user) { double(:user) }
+    let(:user) { double(:user, can_manage_line_items_in_orders_only?: false) }


We might as well improve the spec here by using instance_doubleinstead of double . instance_double actually checks if the stubbed method would exist on an actual instance see: https://rspec.info/features/3-13/rspec-mocks/verifying-doubles/instance-doubles/

Sure, will look into this improvement. 👍🏻

chahmedejaz · 2025-02-17T08:16:43Z

Thanks @rioug for the feedback. Let me incorporate and get back to you.

Ferrum::BrowserError: Argument should belong to the same JavaScript world as target object

chahmedejaz · 2025-03-03T02:22:36Z

Hi @rioug - I've improved the specs now. It's ready for review. Thanks. :)

Edit: Sorry I added specs in the last commit. From future, I'll try to add them in the same commit as the changes

dacook

A big effort, who would have guessed it would be so complicated! It's great to see thorough specs 💪

Some parts of it are hard to understand, because it's adding lots of very specific checks in different places. Eg can_manage_line_items_in_orders_only?

I can't quite get all around it in my head to suggest a better arrangement though. I have several other comments:

app/models/spree/order.rb

app/services/permissions/order.rb

app/helpers/spree/admin/orders_helper.rb

dacook · 2025-03-04T02:47:33Z

app/helpers/spree/admin/orders_helper.rb

@@ -159,6 +159,11 @@ def distributor_allows_order_editing?(order)
          spree_current_user.can_manage_line_items_in_orders_only?
      end

+      def display_value_for_producer(order, value)
+        return value unless distributor_allows_order_editing?(order)


I think the question here is not if distributor_allows_order_editing?, but if "user is distributor?"

If the current user is not an authorised manager of the distributor, then they should not have access to customer details until permitted. It should be easy to check that I think.

In this case, it works, but could be confusing and if the logic changes in the future it might cause a problem.

It's addressed here: 8c73f36

app/models/enterprise.rb

app/models/spree/user.rb

spec/lib/open_food_network/scope_variants_for_search_spec.rb

…aptured in the scoper queries

app/models/spree/user.rb

- we are using OR between two queries here: https://github.com/openfoodfoundation/openfoodnetwork/blob/53ec6621bc30aa86f64440c9b1f9f9357da9a1b1/app/services/search_orders.rb#L31-L32 - so to make it compatible with this, had to revert Throws following error: Relation passed to #or must be structurally compatible. Incompatible values: [:left_outer_joins]

chahmedejaz · 2025-03-05T00:43:48Z

Hi @dacook - I've addressed your comments, please review. Thanks

chahmedejaz · 2025-03-05T00:50:21Z

Some parts of it are hard to understand, because it's adding lots of very specific checks in different places. Eg can_manage_line_items_in_orders_only?
I can't quite get all around it in my head to suggest a better arrangement though. I have several other comments

Yes, I can understand it's confusing a bit 😓 sorry for that.
It's just representing if the user has only the ability to manage line_items in the orders then no other unrelated action should be either viewed or performed in that case.
We are only assigning this ability to the producer who is allowed to edit orders for now.

Please let me know if you have any questions, would love to address. Thanks for digging into this one and your detailed analysis, It's really worth it @dacook ❤️

- this before block was causing multiple nevigation to the index - one from the spec itself, one from here.

dacook

Hi Ahmed, thanks for the explanations and follow-up. I really appreciate you addressing all my questions and concerns.

It's exciting to see the new feature!

As a few things have changed, it would be good to have a second review before testing. But Gaetan is sick and I'm not sure it's practical for Maikel to review the whole thing. So I suggest it's ok to move to testing in this case. What do you think?

chahmedejaz · 2025-03-05T01:29:01Z

Sounds good, @dacook. Moving this to test ready. Thanks.

rioug

Thanks for adding all the specs !

rioug · 2025-03-10T00:03:59Z

spec/system/admin/orders/producer_actions_spec.rb

+      end
+
+
+      def expect_product_change(product, action, expected_qty = 0)


Nice one 👍 It makes the specs so much easier to understand

filipefurtad0 · 2025-03-20T19:56:41Z

Hey @chahmedejaz ,

Nice, this is quite a chunk of work 💪

Going through different scenarios:

Verified that the option to allow producers to edit orders is visible on the enterprise settings

Tooltip is visible:

2.a) Before enabling this option, as the producer, visiting /admin/orders or /admin/bulk_order_management triggers a 401 Unauthorized error, as before:

2.b) after enabling this option, the URLs /admin/orders and /admin/bulk_order_management are accessible by the producer. The following tests were performed, after logging as a producer supplying a hub and with access to order edition:

i) See you can see ONLY orders containing the producer's product in /admin/orders and BOM. Other orders from the hub should not be available. 🟢
ii) You can add, edit (change quantity and weight) and delete products. 🟢
iii) You can't do actions on an order or cancel an order -> it is still possible to ship an order, on the orders page 🔴

iv) You can't see other producer products in the add a product dropdown or on the order details page (unless you manage several producers). 🟢

v) check BOM is pre-filtered if only one producer available 🟢

iv) If customer names in report are disabled, customer names should be hidden in orders and BOM lists 🟢

This seems good to go, except for the fact that the ship button is still visible, in the producer view (test 2.b.iii)). Perhaps best to address this before shipping this feature. Moving back to in Progress.

chahmedejaz · 2025-03-23T11:30:25Z

Thanks @filipefurtad0 for detailed feedback, it's now fixed.

@openfoodfoundation/reviewers it's fixed here: e605229

dacook

👍

filipefurtad0 · 2025-03-24T17:25:29Z

Hey @chahmedejaz,

Thanks for hiding the order shipping option, for the producer, on the orders page 🟢

Apologies, but I have forgotten that we have optional columns on the Bulk Order Management page, for Name and Email:

Also, optional to display, is the customers' Phone too.

I'd argue these should be hidden as well, when the setting for Names in Customer Reports is disabled, for the managing hub. Ping @openfoodfoundation/train-drivers-product-owners.

Since this one is a privacy issue, I think it blocks releasing this feature for now. Moving to In Progress, and sorry again for the back-and-forth. This info was well hidden on my test hub ;-)

RachL · 2025-03-24T22:27:47Z

argh yes, I forgot BOM too! So sorry @chahmedejaz

github-actions bot assigned chahmedejaz Feb 1, 2025

chahmedejaz force-pushed the task/13031-allow-producers-to-edit-orders branch from 764ea78 to ae86821 Compare February 1, 2025 23:56

chahmedejaz force-pushed the task/13031-allow-producers-to-edit-orders branch from 2e12ce7 to a747c88 Compare February 10, 2025 20:46

chahmedejaz added 8 commits February 16, 2025 01:08

add enterprise producers_to_edit_orders setting

d199126

add ability to view supplier products containing orders

f42711c

add ability update supplier line_items in orders

a63dba8

add ability search supplier products in orders

79f0f9e

add bulk_management ability

173110b

restrict page sections from supplier

cf42a64

incorporate show_customer_names_to_suppliers setting

a868ed1

update respective specs

e4ef890

chahmedejaz force-pushed the task/13031-allow-producers-to-edit-orders branch from d9f0454 to e4ef890 Compare February 15, 2025 20:57

chahmedejaz changed the title ~~Task/13031 allow producers to edit orders~~ Allow producer to edit their products on hubs' orders Feb 15, 2025

chahmedejaz marked this pull request as ready for review February 15, 2025 21:07

chahmedejaz commented Feb 16, 2025

View reviewed changes

fix lint issues

90c1863

rioug requested changes Feb 17, 2025

View reviewed changes

add specs

361e7da

chahmedejaz force-pushed the task/13031-allow-producers-to-edit-orders branch from 206acdd to 361e7da Compare March 3, 2025 00:52

fix lint issues

53ec662

chahmedejaz force-pushed the task/13031-allow-producers-to-edit-orders branch from 5858014 to 53ec662 Compare March 3, 2025 01:15

chahmedejaz requested a review from rioug March 3, 2025 01:19

fix flaky spec error:

a77eb11

Ferrum::BrowserError: Argument should belong to the same JavaScript world as target object

dacook self-requested a review March 3, 2025 23:58

dacook requested changes Mar 4, 2025

View reviewed changes

chahmedejaz added 3 commits March 5, 2025 03:27

remove ids usage

887a434

remove subquery for optimization

06ba686

rename distributor_allows_order_editing to filter_by_supplier

8c73f36

chahmedejaz added 2 commits March 5, 2025 03:32

rename is_producer to is_producer_only

8d05b73

implement spree_current_user let! so that user creation doesn't get c…

7d09156

…aptured in the scoper queries

dacook reviewed Mar 4, 2025

View reviewed changes

app/models/spree/user.rb Outdated Show resolved Hide resolved

chahmedejaz added 2 commits March 5, 2025 04:14

fix syntax error

0b941d5

chahmedejaz requested a review from dacook March 5, 2025 00:43

chahmedejaz added 2 commits March 5, 2025 11:56

simplify block

d46cf38

fix buggy spec:

b3877be

- this before block was causing multiple nevigation to the index - one from the spec itself, one from here.

dacook force-pushed the task/13031-allow-producers-to-edit-orders branch from 89cf454 to b3877be Compare March 5, 2025 00:56

dacook approved these changes Mar 5, 2025

View reviewed changes

rioug approved these changes Mar 10, 2025

View reviewed changes

sigmundpetersen added the user facing changes Thes pull requests affect the user experience label Mar 19, 2025

filipefurtad0 self-assigned this Mar 20, 2025

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au labels Mar 20, 2025

hide shipping icons from suppliers

e605229

dacook approved these changes Mar 23, 2025

View reviewed changes

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Mar 24, 2025

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow producer to edit their products on hubs' orders #13113

Allow producer to edit their products on hubs' orders #13113

chahmedejaz commented Feb 1, 2025 •

edited

Loading

chahmedejaz Feb 16, 2025

rioug left a comment

rioug Feb 16, 2025

rioug Feb 16, 2025

chahmedejaz Feb 17, 2025

chahmedejaz commented Feb 17, 2025 •

edited

Loading

chahmedejaz commented Mar 3, 2025 •

edited

Loading

dacook left a comment

dacook Mar 4, 2025 •

edited

Loading

chahmedejaz Mar 4, 2025

chahmedejaz commented Mar 5, 2025

chahmedejaz commented Mar 5, 2025

dacook left a comment

chahmedejaz commented Mar 5, 2025

rioug left a comment

rioug Mar 10, 2025

filipefurtad0 commented Mar 20, 2025

chahmedejaz commented Mar 23, 2025

dacook left a comment

filipefurtad0 commented Mar 24, 2025 •

edited

Loading

RachL commented Mar 24, 2025

		end


		def expect_product_change(product, action, expected_qty = 0)

Allow producer to edit their products on hubs' orders #13113

Are you sure you want to change the base?

Allow producer to edit their products on hubs' orders #13113

Conversation

chahmedejaz commented Feb 1, 2025 • edited Loading

What? Why?

What should we test?

Release notes

chahmedejaz Feb 16, 2025

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

rioug Feb 16, 2025

Choose a reason for hiding this comment

rioug Feb 16, 2025

Choose a reason for hiding this comment

chahmedejaz Feb 17, 2025

Choose a reason for hiding this comment

chahmedejaz commented Feb 17, 2025 • edited Loading

chahmedejaz commented Mar 3, 2025 • edited Loading

dacook left a comment

Choose a reason for hiding this comment

dacook Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

chahmedejaz Mar 4, 2025

Choose a reason for hiding this comment

chahmedejaz commented Mar 5, 2025

chahmedejaz commented Mar 5, 2025

dacook left a comment

Choose a reason for hiding this comment

chahmedejaz commented Mar 5, 2025

rioug left a comment

Choose a reason for hiding this comment

rioug Mar 10, 2025

Choose a reason for hiding this comment

filipefurtad0 commented Mar 20, 2025

chahmedejaz commented Mar 23, 2025

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Mar 24, 2025 • edited Loading

RachL commented Mar 24, 2025

chahmedejaz commented Feb 1, 2025 •

edited

Loading

chahmedejaz commented Feb 17, 2025 •

edited

Loading

chahmedejaz commented Mar 3, 2025 •

edited

Loading

dacook Mar 4, 2025 •

edited

Loading

filipefurtad0 commented Mar 24, 2025 •

edited

Loading