send-shipment-email-optionally #11762

binarygit · 2023-11-05T09:51:27Z

What? Why?

Closes Show pop-in alert when clicking 'ship' button from Orders screen #11602

What should we test?

Scenanio 1:

Login as admin
Visit page /admin/orders
Click on ship
Ascertain that a modal shows up asking you to confirm if a shipment email should be sent
Ascertain when you check the checkbox, a shipment email is sent
Ascertain no shipment email is sent if the checkbox remains unchecked

Scenario 2:

Login as admin
Visit page /admin/orders/<order_number>/edit
Click on ship
Ascertain that a modal shows up asking you to confirm if a shipment email should be sent
Ascertain when you check the checkbox, a shipment email is sent
Ascertain no shipment email is sent if the checkbox remains unchecked

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 · 2023-11-05T23:13:51Z

Moving back to In Dev, as this is still a draft PR

mkllnk

Thank you, this is great. There's just one little thing that I like changed.

mkllnk · 2023-11-22T05:40:59Z

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

+          %div{class: "margin-bottom-30"}
+            = hidden_field_tag :id, order.id
+            = check_box_tag :send_shipment_email
+            = label_tag :send_shipment_email, "Send email confirmation to customer"


This needs a translations.

mkllnk · 2023-11-22T05:44:59Z

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

+      %form
+        = render ConfirmModalComponent.new(id: "ship_order", confirm_reflexes: "click->Admin::OrdersReflex#ship", controller: "orders", reflex: "Admin::Orders#ship") do
+          %div{class: "margin-bottom-30"}
+            %p This will mark the order as Shipped


mkllnk · 2023-11-22T05:45:17Z

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

+            %div{class: "margin-bottom-30"}
+              = hidden_field_tag :id, order.id
+              = check_box_tag :send_shipment_email
+              = label_tag :send_shipment_email, "Send email confirmation to customer"


binarygit · 2023-11-26T18:47:57Z

@mkllnk Thanks for the review. I will work on this tomorrow. Was out sick past couple of days!

binarygit · 2023-11-28T02:59:36Z

@mkllnk Done

rioug

Great work ! 🙏

drummer83 · 2023-12-04T21:09:32Z

Hi @binarygit,
Great to have you back! 🎉
And thank you for working on this issue! 🙏

Here are my test results.

The short version

Unfortunately the specification was incomplete - there is a third place for marking an order as shipped. It's listed as an action in the actions dropdown menu as well. This alone wouldn't be a problem, but unfortunately the behaviour of that action has now changed after this PR - which is why I think that this is a regression we should fix before releasing. Before your PR there was always the email sent without asking, after your PR there is no email sent without asking (using the actions dropdown). Users can't notice and this will cause irritation for sure. We should avoid that.

The long version

Places to test

Button on the orders page
Button on the edit order page
Actions drop down list on the edit order page

Design

Note: I tested with the new design as well as the legacy one, but images are taken from the new one only.

I think we should have the checkbox active as default value because this mimics the behaviour before the PR. Users could be very annoyed by the extra clicks when following their standard workflows. To be confirmed by @openfoodfoundation/train-drivers-product-owners.

Also we have three different designs of the modal now:

Place	Design
Orders page
Edit order page (button)
Edit order page (action)

If not too much work we should unify and in my opinion we should copy this one:

To do:

Make checkbox active as default value.
Add .<br />Are you sure you want to proceed? to the header string.
Rephrase the text: Send a shipment/pick up notification email to the customer..
Use the same design for all three places.

Admin vs. super admin users

Functionality and design are identical for both users. ✔️
However I noticed that as super admin user it takes a long time until the click on the confirmation button is accepted, the modal is closed and the order is marked as shipped. I counted approx. 6 seconds, and that's long enough to try several times and even click cancel - which closes the modal immediately but still marks the order as shipped. Very confusing. Since marking the order as shipped on the /orders page is very quick even for super admins I am hoping that there is room for improvement here. Not sure what is happening during these 6 seconds though.

Action dropdown opens automatically

After marking an order as shipped via the button on the edit order page, the actions dropdown is opened automatically even though the mouse is positioned somewhere else on the screen. This is a (minor) regression as well.
Here is an example (sorry, the mouse is invisible):

Peek.2023-12-04.20-54.mp4

Conclusion

I think we should improve before merging. Would you be ok with that @binarygit?
Also awaiting some feedback from @openfoodfoundation/train-drivers-product-owners especially regarding the default setting of the checkbox..

Thanks for your work on this again! 🙏

RachL · 2023-12-05T08:47:40Z

Yes good call on marking the checkbox active by default 👍 thanks @drummer83

Regarding the designs perhaps we can work on this in a separate PR? If the behavior works, it's a nice little improvement (so many people ship by mistake and then can't cancel anymore). Of course if it's fast to add it, forget me :)

Nice to have you back @binarygit 😍

binarygit · 2023-12-07T04:59:55Z

@drummer83 Thank You so much for the through testing and your remarks. I agree with you. I don't think it will take alot of time to incorporate these changes. I'll work on this this evening and should finish.

drummer83 · 2024-01-06T20:22:07Z

Hi @binarygit,
Thanks for continuing your work on this!

Button on page /orders

Working fine:
- Modal opens ✔️
- Modal can be cancelled ✔️
- Checkbox is active by default ✔️
- Sending email respect the setting of the checkbox ✔️
- Confirming the modal marks the order as shipped ✔️

Button on page /admin/orders/<order_number>/edit

Nothing happens when clicking the button ❌


Error invoking action "click->modal-link#open"

 TypeError: this.application.getControllerForElementAndIdentifier(...) is null
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    __trace__ inline-script-content.js:149
    r inline-script-content.js:162
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    e admin-62bef5ece206ebcb243a.js:2
    d admin-62bef5ece206ebcb243a.js:2
    M admin-62bef5ece206ebcb243a.js:2
    k admin-62bef5ece206ebcb243a.js:2
    i admin-62bef5ece206ebcb243a.js:2
    s admin-62bef5ece206ebcb243a.js:2
    promise callback*i admin-62bef5ece206ebcb243a.js:2
    s admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
 

 
Object { identifier: "modal-link", controller: {…}, element: button.ship.button.icon-arrow-right
, index: 0, event: click }
[console-breadcrumbs.js:33:15](https://d2wy8f7a9ursnm.cloudfront.net/plugin-console-breadcrumbs/console-breadcrumbs.js)
    r console-breadcrumbs.js:33
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    __trace__ inline-script-content.js:149
    (Async: EventListener.handleEvent)
    r inline-script-content.js:162
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    e admin-62bef5ece206ebcb243a.js:2
    d admin-62bef5ece206ebcb243a.js:2
    M admin-62bef5ece206ebcb243a.js:2
    k admin-62bef5ece206ebcb243a.js:2
    i admin-62bef5ece206ebcb243a.js:2
    s admin-62bef5ece206ebcb243a.js:2
    (Async: promise callback)
    i admin-62bef5ece206ebcb243a.js:2
    s admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    value admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    n admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2
    <anonymous> admin-62bef5ece206ebcb243a.js:2

Action from dropdown menu

Nothing happens when clicking 'Ship order' ❌

Conclusion

I'm really sorry. I think we have to look at the two failing cases. 😢

binarygit · 2024-01-07T15:44:59Z

Thanks for testing this @drummer83 . I'll look into what's happening and push a fix.

…it page

binarygit · 2024-01-14T10:46:44Z

@drummer83 Could you please test this again? The recent commit fixes the issues you mentioned above and also adds tests!

The meat of the commit is adding the tests which is why I am not re-requesting a review.

Also I think the whole commit history is shown as "new" because I rebased latest changes from master and there was a conflict in one file which I resolved.

drummer83 · 2024-01-18T11:01:45Z

Hi @binarygit,
I have tested this again! 👍

Button on page /orders

Working fine:
- Modal opens ✔️
- Modal can be cancelled (clicking 'Cancel' or outside of the modal) ✔️
- Checkbox is active by default ✔️
- Sending email respects the setting of the checkbox ✔️
- Confirming the modal marks the order as shipped ✔️
Remaining issue:
- Ship icon disappears on hovering (not related to this PR) ❌ -> [BUU][Orders] Ship icon is invisible on hovering #11692

Button on page /admin/orders/<order_number>/edit

Working fine:
- Modal opens ✔️
- Modal can be cancelled (clicking 'Cancel' or outside of the modal) ✔️
- Checkbox is active by default ✔️
- Sending email respects the setting of the checkbox ✔️
- Confirming the modal marks the order as shipped ✔️
Remaining issues:
- After confirming, the action dropdown is opened automatically ❌ -> [BUU][Orders] Ship icon is invisible on hovering #11692

Action from dropdown menu

Working fine:
- Modal opens ✔️
- Modal can be cancelled (clicking 'Cancel' or outside of the modal) ✔️
- Checkbox is active by default ✔️
- Sending email respects the setting of the checkbox ✔️
- Confirming the modal marks the order as shipped ✔️
Remaining issues:
- After confirming, the action dropdown is opened automatically ❌ -> [Order details] Actions dropdown opens automatically after shipping the order #12063

Conclusion

Great! We are there now! 🥳
Thank you very much for your patience and for pushing this over the line!
I'm merging this one and will create issue for the little issues which are not related to your PR! 🚀
Thanks again!

binarygit force-pushed the send-shipment-email-optionally branch from ab03a22 to 3a99a57 Compare November 5, 2023 09:23

binarygit force-pushed the send-shipment-email-optionally branch 4 times, most recently from 09be01e to 6fbcbec Compare November 16, 2023 14:16

binarygit force-pushed the send-shipment-email-optionally branch from 6fbcbec to 257cffc Compare November 21, 2023 08:18

binarygit marked this pull request as ready for review November 21, 2023 09:15

sigmundpetersen assigned binarygit Nov 21, 2023

mkllnk requested changes Nov 22, 2023

View reviewed changes

mkllnk reviewed Nov 22, 2023

View reviewed changes

sigmundpetersen requested a review from mkllnk November 28, 2023 07:55

mkllnk approved these changes Nov 29, 2023

View reviewed changes

rioug approved these changes Dec 1, 2023

View reviewed changes

drummer83 self-assigned this Dec 4, 2023

drummer83 added no-staging-UK A tag which does not trigger deployments, indicating a server is being used pr-staged-uk staging.openfoodnetwork.org.uk and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used labels Dec 4, 2023

drummer83 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Dec 4, 2023

drummer83 removed their assignment Dec 6, 2023

drummer83 removed the feedback-needed label Dec 6, 2023

binarygit force-pushed the send-shipment-email-optionally branch from 059de87 to 007c95b Compare December 9, 2023 11:10

binarygit force-pushed the send-shipment-email-optionally branch from 007c95b to 8114d6e Compare December 20, 2023 14:13

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 6, 2024

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 6, 2024

drummer83 removed their assignment Jan 6, 2024

binarygit added 13 commits January 14, 2024 15:05

send-shipment-email-optionally

95a5115

send shipment email optionally from orders#edit page

5e45d3b

Remove redundant function

99f4c47

Add translations

5fe246e

Send shipment email by default

aa7a4fb

Create ShipOrder component

e83fd5b

Show same dialog box when shipping order through the actions dropdown

267e749

Change displayed message

36c5d7c

Fix tests and rubocop warnings

75ca1dd

Use transaltion with HTML in it by adding html suffix

6e00a34

Use cable_ready.replace instead of morphing after order is captured

5b99c26

Bugfix: Link individual send shipment button to its corresponding form

bc57074

Associate each label with its checkbox

746b521

binarygit force-pushed the send-shipment-email-optionally branch from ecd2f9f to e7c806b Compare January 14, 2024 09:26

Provide correct modal-link-target-value and write tests for orders#ed…

e865f27

…it page

binarygit force-pushed the send-shipment-email-optionally branch from e7c806b to e865f27 Compare January 14, 2024 09:48

rioug approved these changes Jan 14, 2024

View reviewed changes

mkllnk approved these changes Jan 14, 2024

View reviewed changes

drummer83 self-assigned this Jan 18, 2024

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 18, 2024

drummer83 merged commit bed4057 into openfoodfoundation:master Jan 18, 2024
52 checks passed

drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 18, 2024

mkllnk added the user facing changes Thes pull requests affect the user experience label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send-shipment-email-optionally #11762

send-shipment-email-optionally #11762

binarygit commented Nov 5, 2023 •

edited

rioug commented Nov 5, 2023

mkllnk left a comment

mkllnk Nov 22, 2023

mkllnk Nov 22, 2023

mkllnk Nov 22, 2023

binarygit commented Nov 26, 2023

binarygit commented Nov 28, 2023

rioug left a comment

drummer83 commented Dec 4, 2023 •

edited

RachL commented Dec 5, 2023

binarygit commented Dec 7, 2023

drummer83 commented Jan 6, 2024

binarygit commented Jan 7, 2024

binarygit commented Jan 14, 2024 •

edited

drummer83 commented Jan 18, 2024 •

edited

send-shipment-email-optionally #11762

send-shipment-email-optionally #11762

Conversation

binarygit commented Nov 5, 2023 • edited

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

rioug commented Nov 5, 2023

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Nov 22, 2023

Choose a reason for hiding this comment

mkllnk Nov 22, 2023

Choose a reason for hiding this comment

mkllnk Nov 22, 2023

Choose a reason for hiding this comment

binarygit commented Nov 26, 2023

binarygit commented Nov 28, 2023

rioug left a comment

Choose a reason for hiding this comment

drummer83 commented Dec 4, 2023 • edited

The short version

The long version

Places to test

Design

Admin vs. super admin users

Action dropdown opens automatically

Conclusion

RachL commented Dec 5, 2023

binarygit commented Dec 7, 2023

drummer83 commented Jan 6, 2024

Button on page /orders

Button on page /admin/orders/<order_number>/edit

Action from dropdown menu

Conclusion

binarygit commented Jan 7, 2024

binarygit commented Jan 14, 2024 • edited

drummer83 commented Jan 18, 2024 • edited

Button on page /orders

Button on page /admin/orders/<order_number>/edit

Action from dropdown menu

Conclusion

binarygit commented Nov 5, 2023 •

edited

drummer83 commented Dec 4, 2023 •

edited

binarygit commented Jan 14, 2024 •

edited

drummer83 commented Jan 18, 2024 •

edited