Fix Ship Order menu choice not working #12376

cyrillefr · 2024-04-12T13:33:16Z

What? Why?

Closes [Admin] 'Ship Order' only works on page Order Details #12369

Error when trying to ship an order when everywhere in admin/orders/RXX/customer, admin/orders/RXXX/payments, admin/orders/RXXXX/adjustments, admin/orders/RXXXXX/return_authorizations
Reason: there were not the ShipOrderComponent which enables a modal for shipping order. The same component this present when in the admin/orders/R52384XXX/edit page

added ShipOrderComponent in the view that handles the dropdown (_order_links.html.haml)
updated spec to test for a specific case

What should we test?

As an admin, visit admin/orders.
Choose an item that has yet to be shipped. (the ones that got to be captured)
Edit it -> admin/orders/R52384XXX/edit
Click anything a part 'ORDER DETAILS', eg CUSTOMER DETAILS or ADJUSTMENTS
Select 'Ship Order' from the actions dropdown menu.
the modal should appear without errors and be functional
clicking on 'Confirm' should put the item in the shipped state.
By going/clicking on 'Order Details', it should be displayed 'shipped'

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

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

Dependencies

Documentation updates

rioug · 2024-04-15T23:29:01Z

app/views/spree/admin/shared/_order_links.html.haml

@@ -19,3 +21,6 @@
              %span=link[:name]

 = render 'spree/admin/shared/custom-confirm'
+- if shipment_ready


I am wondering if we could move the shipment_ready logic in an helper ? so we could do if order_shipment_ready?(@order)

I am not a big fan of setting variable in the view, in this case it's not quite clear what's why shipment_ready is set to true

Yes, that's a good idea.

I agree, I might have been a bit hurry. I have put the logic in the order_helpers.rb

mkllnk · 2024-04-16T01:49:14Z

app/views/spree/admin/shared/_order_links.html.haml

@@ -19,3 +21,6 @@
              %span=link[:name]

 = render 'spree/admin/shared/custom-confirm'
+- if shipment_ready


Yes, that's a good idea.

filipefurtad0 · 2024-04-16T15:28:02Z

Hey @cyrillefr ,

I've submitted a PR (here) with a spec reproducing this regression (it's currently in code review). In case you find it useful, please feel free to cherry pick it, for this PR 👍

rioug

Looks good now !

cyrillefr · 2024-04-17T14:57:58Z

Hello @filipefurtad0 ,

Thanks for the offer, I think your PR will be complementary to mine. Coverage will be top.
It 's about to be merged, so I feel a bit insecure to change it now :)

filipefurtad0 · 2024-04-17T16:21:10Z

Thanks for the offer, I think your PR will be complementary to mine. Coverage will be top.
It 's about to be merged, so I feel a bit insecure to change it now :)

Thanks @cyrillefr - that's perfect as well: after merging, I can remove the pending from the tests 👍

mkllnk · 2024-04-17T22:38:10Z

Oh, I just merged the other PR. So we have to resolve it here now.

mkllnk · 2024-04-17T22:55:05Z

Hm, the new specs seem to be failing. I shouldn't have merged them without testing against the solution.

mkllnk

I rebased the branch to resolve the conflict on the spec. I also changed Filipe's spec with bits from @cyrillefr's spec but I think that there's a bug.

Can you have another look?

mkllnk · 2024-04-17T23:37:13Z

spec/system/admin/order_spec.rb

+
+                  # the best & easiest way to close the modal without calling all the JS
+                  find_button("Cancel").click


This seems to be a bug that needs to be resolved. Testing locally, the modal didn't close for me in dev environment either.

I deleted this part to work with the shared examples. In the new version, it closes well after running examples dozens of time. Hope it will be the same in your local and dev.

mkllnk · 2024-04-17T23:37:44Z

spec/system/admin/order_spec.rb

+        context 'when not on the Order Details sub section' do
+          before do
+            click_link 'Customer Details'
+          end
+          it 'can ship order too' do
+            find('.ofn-drop-down').click
+            click_link 'Ship Order'
+
+            within ".reveal-modal" do
+              expect(page).to have_checked_field('Send a shipment/pick up ' \
+                                                 'notification email to the customer.')
+              # no test of enqueued job since it can cause failures
+              # if the remainder of the spec is too fasr
+              find_button('Confirm').click
+            end
+
+            # the best & easiest way to close the modal without calling all the JS
+            find_button("Cancel").click
+            expect(order.reload.shipped?).to be true
+            click_link('Order Details')
+            expect(page).to have_text 'SHIPPED'
+          end


I think that this is now covered by the above shared examples. I just left it here for reference.

Yes, it is now included in the shared examples, so I deleted this part. Also I modified a bit the shared examples, see below.

cyrillefr · 2024-04-21T09:09:57Z

Hello @mkllnk ,

some words about my last changes.
I have dropped my own spec to work with the shared examples made by @filipefurtad0.

I have added a click to the 'Order Details' part to check for the shipped status (it only writes there).
To do so, I need to close the modal, which does not closes itself.

I then had a problem, from time to time: the mailing job was not enqueued.
I think it is because when the modal closes, the mailing job did not have enough time to be enqueued.
A little sleep(0.5) solved this flakyness.

But it is not a good practice, so I have looked for another solution.
Which is to check at the end of the example if the job was enqueued. It requires some more precision about what kind of job to check, but is works well.
I have run the shared examples 10 times (100 in total) and they pass.
So I am confident there is no flakyness.

mkllnk · 2024-04-23T01:02:03Z

I need to close the modal, which does not closes itself.

But that's a bug, right? Shouldn't the modal close when you choose an action? @filipefurtad0

filipefurtad0 · 2024-04-23T22:21:13Z

I need to close the modal, which does not closes itself.

But that's a bug, right? Shouldn't the modal close when you choose an action? @filipefurtad0

I've staged the PR, and I think you mean the confirmation modal - right? The dropdown closes when clicked, but the confirmation modal indeed requires a click/action from the user. Is this what you refer to @cyrillefr ? If so I think it is the correct behavior.

mkllnk · 2024-04-24T00:10:06Z

I meant this confirmation modal:

After clicking CONFIRM, the modal doesn't close for me. Why should it stay open?

cyrillefr · 2024-04-24T06:59:20Z

Hello @filipefurtad0 ,

Yes the modal I am referring to is the one on the screenshot, when you edit an order.

filipefurtad0 · 2024-04-24T11:25:37Z

I meant this confirmation modal:
Yes the modal I am referring to is the one on the screenshot, when you edit an order.

Ok, I see it now. I've pulled the code from this PR and compared it with master: indeed, in master the confirmation modal closes after selecting an option; this is not the case within this PR.

I agree with @mkllnk: we should keep the current behavior (in master), i.e., the modal should close after selecting one of the options.

cyrillefr · 2024-04-24T15:26:52Z

Hello @filipefurtad0 , I have checked and here are the details:

dev/local mode
- in this branch, the modal closes itself - only - if the subpage is 'Order Details'.
- if another sub-page, it does not
testing mode
- subpage 'Order Details' the modal does not close itself at first, you need to click twice ...which differs from the dev/local.
- does not closes in the other subpages

Which means this modal is dependent of the subpage.

@mkllnk , I 'll go check and update as soon as possible.

cyrillefr · 2024-04-24T22:02:50Z

Hello @mkllnk ,

Now there is no need to close the modal manually in the specs.
I had noticed that in app/reflex/admin/orders_reflex.rb there was a kind of a choice between edit (the page of concern) and the index (rows) page. But did not pay enough attention.
What happens here is that there is a redirection to the classical Rails controllers, now for every sub-page (before that, only for the edit page).
So the behaviour is the same now for every sub-page.
But ... the modal does still not close itself (there is a need of a modal:close call, like at line 72:
cable_ready.dispatch_event(name: "modal:close") .

But because there is a redirection to a classical Rails controller, the page is reloaded and the reload make the modal go away.

Also , I would need a rerun of the checks since the specs in error works fine locally.

mkllnk

Ah, good. Page reload is fine.

mkllnk · 2024-05-01T23:41:34Z

app/reflexes/admin/orders_reflex.rb

+  module Constants
+    PATHS = %w[edit customer payments adjustments invoices return_authorizations].freeze
+  end


I don't think that you need an additional module just to declare a constant.

I agree with you, but there is a 100 lines limit for Class size, so I had to do this not to raise the rubocop offense.

Oh, this is a good example for style rules producing worse code. Rubocop is supposed to help us. If its recommendation is stupid, we need to do something else.

I agree with Rubocop that the class is getting too big. Adding a module doesn't help though. We could move some code into other files like an InvoiceReflex, for example. But since we are going to shift away from reflexes, we can just add it to the rubocop todo file.

So, I added a new line on the rubocop todo file :)

- Front End ShipOrderComponent was missed for the menu to work - updated spec to test for a specific case

- deletes local variables in view - factor logic in helper

- removes the pending - add the click to go to the 'Order Details' page to check for the 'shipped' status - from enqueued to have_been_enqueued so the spec is flaky-free

- returns to the same processing for customer, invoices etc. that edit - need a bit of sleep in spec bc 2 tasks are asynchroneous

- bypass rubocop class lines limit

filipefurtad0 · 2024-05-15T16:21:35Z

Hey @cyrillefr ,

Awesome, thanks so much for your work on this and the thorough description.

I've tested both as superadmin and hub admin, selecting the Ship Order from the Actions drop down, on the order edit page, for several orders in shipment_state = ready, on the tabs:

Customer Details
Order Details
Payments
Adjustments

In all cases, the shipping state changed to shipped, after confirming within the modal; Emails arrived as expected.

Great to finally have this one through 👏 👏 👏
Merging.

dacook · 2024-05-16T06:26:42Z

It looks like this fixes a user-facing bug, so I'm updating the label. Please correct me if wrong!

rioug reviewed Apr 15, 2024

View reviewed changes

mkllnk requested changes Apr 16, 2024

View reviewed changes

sigmundpetersen assigned cyrillefr Apr 16, 2024

filipefurtad0 mentioned this pull request Apr 16, 2024

Adds regression spec as shared example, for order edit subsections #12370

Merged

4 tasks

mkllnk approved these changes Apr 16, 2024

View reviewed changes

rioug approved these changes Apr 16, 2024

View reviewed changes

rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Apr 16, 2024

mkllnk force-pushed the MakeShipOrderButtonWorksOnPageOrderDetails branch from ee1a806 to 4260047 Compare April 17, 2024 23:36

mkllnk requested changes Apr 17, 2024

View reviewed changes

cyrillefr force-pushed the MakeShipOrderButtonWorksOnPageOrderDetails branch from 4260047 to 320b551 Compare April 21, 2024 08:42

filipefurtad0 self-assigned this Apr 23, 2024

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Apr 23, 2024

cyrillefr force-pushed the MakeShipOrderButtonWorksOnPageOrderDetails branch from dceea4e to 37616c6 Compare April 24, 2024 21:43

mkllnk approved these changes May 1, 2024

View reviewed changes

cyrillefr added 2 commits May 7, 2024 19:15

Fix Ship Order menu choice not working

901c29c

- Front End ShipOrderComponent was missed for the menu to work - updated spec to test for a specific case

Fix 'Ship Order': requested changes

ba68f4b

- deletes local variables in view - factor logic in helper

cyrillefr added 3 commits May 7, 2024 19:15

Fix 'Ship Order': integrating non regression spec

34bb05e

- removes the pending - add the click to go to the 'Order Details' page to check for the 'shipped' status - from enqueued to have_been_enqueued so the spec is flaky-free

Fix Ship Order Button: modal should close itself

271d50e

- returns to the same processing for customer, invoices etc. that edit - need a bit of sleep in spec bc 2 tasks are asynchroneous

Fix Ship Order Button: requested changes

6575ad7

- bypass rubocop class lines limit

sigmundpetersen force-pushed the MakeShipOrderButtonWorksOnPageOrderDetails branch from 9447b95 to 6575ad7 Compare May 7, 2024 17:15

rioug approved these changes May 7, 2024

View reviewed changes

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr labels May 15, 2024

filipefurtad0 removed the pr-staged-au staging.openfoodnetwork.org.au label May 15, 2024

filipefurtad0 merged commit 59e04cc into openfoodfoundation:master May 15, 2024
54 of 56 checks passed

dacook added user facing changes Thes pull requests affect the user experience and removed technical changes only These pull requests do not contain user facing changes and are grouped in release notes labels May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ship Order menu choice not working #12376

Fix Ship Order menu choice not working #12376

cyrillefr commented Apr 12, 2024

rioug Apr 15, 2024

mkllnk Apr 16, 2024

cyrillefr Apr 16, 2024

mkllnk Apr 16, 2024

filipefurtad0 commented Apr 16, 2024

rioug left a comment

cyrillefr commented Apr 17, 2024

filipefurtad0 commented Apr 17, 2024

mkllnk commented Apr 17, 2024

mkllnk commented Apr 17, 2024

mkllnk left a comment

mkllnk Apr 17, 2024

cyrillefr Apr 21, 2024

mkllnk Apr 17, 2024

cyrillefr Apr 21, 2024

cyrillefr commented Apr 21, 2024

mkllnk commented Apr 23, 2024

filipefurtad0 commented Apr 23, 2024

mkllnk commented Apr 24, 2024

cyrillefr commented Apr 24, 2024

filipefurtad0 commented Apr 24, 2024

cyrillefr commented Apr 24, 2024 •

edited

cyrillefr commented Apr 24, 2024

mkllnk left a comment

mkllnk May 1, 2024

cyrillefr May 2, 2024

mkllnk May 7, 2024

cyrillefr May 7, 2024

filipefurtad0 commented May 15, 2024

dacook commented May 16, 2024


		# the best & easiest way to close the modal without calling all the JS
		find_button("Cancel").click

Fix Ship Order menu choice not working #12376

Fix Ship Order menu choice not working #12376

Conversation

cyrillefr commented Apr 12, 2024

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

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

filipefurtad0 commented Apr 16, 2024

rioug left a comment

Choose a reason for hiding this comment

cyrillefr commented Apr 17, 2024

filipefurtad0 commented Apr 17, 2024

mkllnk commented Apr 17, 2024

mkllnk commented Apr 17, 2024

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

cyrillefr commented Apr 21, 2024

mkllnk commented Apr 23, 2024

filipefurtad0 commented Apr 23, 2024

mkllnk commented Apr 24, 2024

cyrillefr commented Apr 24, 2024

filipefurtad0 commented Apr 24, 2024

cyrillefr commented Apr 24, 2024 • edited

cyrillefr commented Apr 24, 2024

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

filipefurtad0 commented May 15, 2024

dacook commented May 16, 2024

cyrillefr commented Apr 24, 2024 •

edited