Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653

wandji20 · 2024-07-09T17:08:23Z

What? Why?

Closes Display a warning message when changing the date range of an OC with linked orders #11613

To enable a warning modal when editing an order cycle with a linked schedule, I added

A link modal in the order cycle edit template (trigger activated in save bar when date time range value changes)
A stimulus controller to monitor changes in the date time range values and display appropriate save bar buttons.

What should we test?

Editing a not-linked schedule order cycle should work as before
In case an order cycle has a linked schedule, if a change is detected in the date time range values (orders open at and close at) the user will see a warning modal on an attempt to 'save', 'save and next', or 'save and back to list'.
However, on confirming the modal, the corresponding user action will be triggered.
Visit ... page.
http://localhost:3000/admin/order_cycles/:id/edit

Release notes

N/A

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

User facing changes
Technical changes only

Add warning modal to order cycle with attached schedule general setting form

Dependencies

N/A

Documentation updates

N/A

RachL · 2024-07-19T11:49:03Z

@wandji20 same here, it looks like the PR needs a rebase

rioug

Nice one @wandji20 👍 , I like that you found a way to use stimulusjs and have angularjs communicate with the stimulus controller.
Ideally we would remove angulajs all together but that's out of scope for this.

I think we can improve your solution a little, could you have a look at my comments ?

rioug · 2024-07-22T00:42:15Z

app/webpacker/controllers/order_cycle_controller.js

+    if(!this.hasScheduleValue) return;
+
+    // Check that datetime input value has a change
+    const dirty = this.inputTargets.some(ele =>


TIL about some()

rioug · 2024-07-22T01:12:48Z

app/webpacker/controllers/order_cycle_controller.js

+    });
+  }
+
+  updateCallback(data) {


I see we are using this as a way to update initValsValue when the order cycle is update via angular 👍 nice trick to allow stimmulus and angular to communicate !

Could you add a comment to explain why we are using this ?

rioug · 2024-07-22T01:12:50Z

app/webpacker/controllers/order_cycle_controller.js

+    this.modalConfirmTargets.forEach(ele => {
+      if (e.target.getAttribute('data-target') === ele.getAttribute('data-request')) {
+        ele.style.display = 'unset';
+      } else {
+        ele.style.display = 'none';
+      }
+    });


It's a bit hard to follow what's happening, but if understand correctly you are using the data-target to display the correct button the modal, right ?
I think you could just pass a parameter to updateModalConfirmButton() to achieve the same thing, and I believe it would make the intent of the function easier to understand.
Something like :

Suggested change

this.modalConfirmTargets.forEach(ele => {

if (e.target.getAttribute('data-target') === ele.getAttribute('data-request')) {

ele.style.display = 'unset';

} else {

ele.style.display = 'none';

}

});

this.modalConfirmTargets.forEach(ele => {

if (e.params.request === ele.getAttribute('data-request')) {

ele.style.display = 'unset';

} else {

ele.style.display = 'none';

}

});

Then you can call it like so :

%input.red{ type: "button", value: t('.save_and_next'), "ng-disabled": "!order_cycle_form.$dirty || order_cycle_form.$invalid", data: { 'action': 'click->modal-link#open click->order-cycle#updateModalConfirmButton', 'order-cycle-request-param': 'saveAndNext'} }

rioug · 2024-07-22T01:17:02Z

app/views/admin/order_cycles/edit.html.haml

+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'save' }, "ng-click": "submit($event, null)" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')
+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'saveAndNext' }, "ng-click": "submit($event, '#{main_app.admin_order_cycle_incoming_path(@order_cycle)}')" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')
+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'saveAndBack' }, "ng-click": "submit($event, '#{main_app.admin_order_cycles_path}')" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')


I am not a big fan of having a mix of stimulus and angular actions on the same button, but can't really think of another way of doing it ?

I agree, it doesn't look right, but not sure of a better way. At least it's a way to continue the transition to Stimulus, away from Angular.

dacook

Thanks for working on this, it looks like a big task!

In addition to Gaetan's comments, I note in the issue that it depends if any orders are made in the order cycle, it's not directly about "schedules" which are a different concept, so I think that will need changing.

Secondly, I think there's two ways to solve it. We could validate on the client-side (as you have), or on the server-side, then trigger the modal. I think server-side might have been simpler, and it has the advantage of checking up-to-date information from the database (for example an order could have been made after the page loaded).
Still, there's advantages to validating client-side, it's nice and quick.

I have a couple of suggestions to simplify it though, which I hope will make the code easier to maintain in the long run. Could you please have a look?

app/views/admin/order_cycles/edit.html.haml

dacook · 2024-07-22T01:23:10Z

app/webpacker/controllers/order_cycle_controller.js

+    const dirty = this.inputTargets.some(ele =>
+      new Date(this.initValsValue[`${ele.name}`]).getTime() !== new Date(ele.value).getTime());


Instead of having to pass the current date in as a variable, we can actually ask the DOM by using ele.defaultValue. See example here:

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/webpacker/controllers/bulk_form_controller.js#L163

Then you won't need to add the initVals values and the HTML can be cleaner.

app/webpacker/css/admin/order_cycles.scss

spec/system/admin/order_cycles/edit_spec.rb

dacook · 2024-07-22T01:33:06Z

app/views/admin/order_cycles/edit.html.haml

+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'save' }, "ng-click": "submit($event, null)" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')
+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'saveAndNext' }, "ng-click": "submit($event, '#{main_app.admin_order_cycle_incoming_path(@order_cycle)}')" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')
+          %button.button.secondary#modal-confirm{ type: "button", 'data-action': 'click->modal#close', style: 'display: none;', data: { 'order-cycle-target': 'modalConfirm', request: 'saveAndBack' }, "ng-click": "submit($event, '#{main_app.admin_order_cycles_path}')" }
+            = t('admin.order_cycles.edit.linked_schedule_warning_modal.proceed')


I agree, it doesn't look right, but not sure of a better way. At least it's a way to continue the transition to Stimulus, away from Angular.

wandji20 · 2024-07-24T19:23:00Z

Hi
I decided to give a shot with validations on the server side which seems less complicated and clunky.

dacook · 2024-07-25T11:23:49Z

Thanks sorry I wasn't able to review this week; we will review on Monday.

rioug

Thanks @wandji20 ! The server side validation looks good, there just a couple of things to tweak and we should be good to go.

app/controllers/admin/order_cycles_controller.rb

app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee

dacook

Thanks Wandji, I also have some comments, and request for some explanation

app/controllers/admin/order_cycles_controller.rb

dacook · 2024-07-29T01:20:47Z

app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee

+        if data.trigger_action
+          StatusMessage.display 'notice', "You have unsaved changes"
+          $("#linked-order-warning-modal button[data-trigger-action=#{data.trigger_action}]").css({ display: 'block' });
+          $('.warning-modal button.modal-target-trigger').trigger('click');
+          return;


I haven't yet figured out how this works, but I can see an untranslated string "You have unsaved changes" which will be a problem.

I have to confess I still don't see what this is for.. could you please explain?

Sure @dacook
So what happens here is that I

Set the status display message to show unsaved changes
Why?
Before the request is made, we set the status message to Saving, and we still get the Saving status message after the warning modal is opened.
And I agree, this should be translated.

Then find and display the appropriate confirm button from the warning modal.

app/views/admin/order_cycles/_date_time_warning_modal_content.html.haml

dacook · 2024-07-29T01:34:04Z

app/views/admin/order_cycles/_date_time_warning_modal_content.html.haml

@@ -0,0 +1,14 @@
+.modal-body.flex-column-gap-1


Is this rule in use? .flex-column-gap-1
It looks similar to others so I suspect isn't necessary

Suggested change

.modal-body.flex-column-gap-1

.modal-body

Thanks for noticing this one
I am using .flex-column.gap-1 so will remove .modal-body

app/models/order_cycle.rb

app/views/admin/order_cycles/edit.html.haml

…ribute change and trigger warning modal [OFN-11613]

dacook

Thanks, that looks like it covers it well.
I can see that we already had spec files that cover simple and complex order cycles maybe you could have added to those.
But I guess it also makes sense to group these similar specs together in one file like you have. 🤷

dacook · 2024-08-12T23:36:34Z

spec/system/admin/order_cycles/edit_spec.rb

+        find('#order_cycle_orders_close_at').click
+        within(".flatpickr-calendar.open") do
+          expect(page).to have_selector '.shortcut-buttons-flatpickr-buttons'
+          select_datetime_from_datepicker Time.zone.parse("2024-03-30 00:00")
+          find("button", text: "Close").click
+        end


I'm surprised to see that we don't have a helper method to handle this, it seems like a common action.

drummer83 · 2024-08-13T17:16:42Z

Hi @wandji20,
Thank you for this PR! Here are my testing notes.

Test cases

The popup: ✔️

When changing the date range of an OC which has linked orders, display a popup, for instance: "Orders are linked to this order cycle. If you wish to create a new order cycle, it is recommended to duplicate the order cycle first and then change the dates." ✔️
The popup is independent from added schedules. ✔️
The popup is shown when clicking the 'save', 'save and next' (complex order cycle) and 'save and back to list' (simple order cycle) buttons. ✔️
The popup is shown when changing start date only, end date only, or both. ✔️
The popup is shown on the simple and on the complex order cycle editing page. ✔️
The popup is shown for admin and super admin users. ✔️
The popup is shown for past, active, and future order cycles. ✔️
The popup is only shown for order cycles that have orders linked to it. ✔️
The popup has two buttons: "Proceed anyway" & "Cancel" ✔️
- Proceed anyway => the saving process goes according to what the user has selected (saving and staying on the form, or saving and going to the next page, or saving and going back to the list). ✔️
- Cancel should move the user back to the list (where the duplicate button is visible) ❌
  The popup is closed, but the user stays on the same page and the changed information is still there.

Notes

The popup only works on the edit page of the OC (as specified), but dates can be adjusted in the OC list as well. ❓
Warning is displayed as soon as an order is in 'cart' state, it's not waiting for 'complete'. ❓

Conclusion

Everything is working well! 💪
I only found one mismatch between the specification and the implementation, which is the behaviour after clicking 'Cancel'. Simply closing itself is what usually happens to a popup when clicking cancel - so the implementation is according to some kind of standard. However, the user has to make the additional click on the cancel button on the order cycles edit page to return to the list of order cycles, all other changes will be lost.
@openfoodfoundation/train-drivers-product-owners Can you please comment whether we can move forward as is, whether we need an adjustment of the implementation, or open a follow-up issue. Thanks!
@openfoodfoundation/train-drivers-product-owners Can you also comment on the findings in the 'Notes' section, please. Thanks!
Adding the feedback-needed label for now.

Thanks again! 🙏

wandji20 · 2024-08-13T17:48:55Z

Thanks for testing @drummer83
Oops, I completely missed the cancel button interaction.
I am unsure how it works from here, but I would not mind addressing these issues in another PR.

drummer83 · 2024-08-14T13:19:26Z

Let's see what @RachL 's opinion is on this. 🙏

RachL · 2024-08-14T13:38:27Z

Apologies for the late answer here, thanks Konrad for the reminder.

thanks @wandji20 for working on this one, it will be life changing for some users!

And thanks @drummer83 for the detailed testing, as always 😍

My proposal would go for merging as is and handling :

behavior of cancel button
displaying the popup on the OC list

in one or 2 follow-up PRs / issues. @wandji20 would you be available to work on both? maybe the OC list should be more a priority than the cancel button IMO.

Regarding the fact that the order is in cart state and not complete state, I see an advantage: at least if for whatever reason the user has several OC open at the same time, maybe orders in cart state can be completed 🤷‍♀️ Maybe this is an edge case, so anyway let's see with user feedback how to proceed on this one.

any thoughts?

drummer83 · 2024-08-14T16:14:27Z

I agree, the topic around the order state 'cart' is an edge case. No need for action here, I think.
I will open issues for the two other topics remaining.
Merging! 💥

drummer83 · 2024-08-14T16:27:52Z

Created #12774 and #12775 to follow up.

github-actions bot assigned wandji20 Jul 9, 2024

wandji20 force-pushed the wb-OFN-11613 branch 5 times, most recently from 84d0f65 to ddc0766 Compare July 11, 2024 12:23

wandji20 mentioned this pull request Jul 19, 2024

Display a warning message when changing the date range of an OC with linked orders #11613

Closed

wandji20 force-pushed the wb-OFN-11613 branch 2 times, most recently from 05935ae to e7c7b54 Compare July 19, 2024 14:01

rioug added the user facing changes Thes pull requests affect the user experience label Jul 22, 2024

dacook self-requested a review July 22, 2024 00:46

rioug requested changes Jul 22, 2024

View reviewed changes

dacook requested changes Jul 22, 2024

View reviewed changes

wandji20 force-pushed the wb-OFN-11613 branch 3 times, most recently from e106f40 to 30c3e8f Compare July 24, 2024 19:06

wandji20 requested review from dacook and rioug July 24, 2024 19:23

rioug requested changes Jul 29, 2024

View reviewed changes

app/controllers/admin/order_cycles_controller.rb Outdated Show resolved Hide resolved

app/controllers/admin/order_cycles_controller.rb Outdated Show resolved Hide resolved

app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee Outdated Show resolved Hide resolved

dacook reviewed Jul 29, 2024

View reviewed changes

wandji20 force-pushed the wb-OFN-11613 branch 7 times, most recently from d911d35 to f55642e Compare August 1, 2024 08:09

wandji20 and others added 3 commits August 12, 2024 09:05

Add stimulus controler to monitor order cycle status message data att…

6a438a0

…ribute change and trigger warning modal [OFN-11613]

Refactor and update order cycle form controller [OFN-11613]

ad3e772

Remoce left over console.log

43d983c

wandji20 force-pushed the wb-OFN-11613 branch 7 times, most recently from e97fbf2 to 6feab1b Compare August 12, 2024 21:09

Include order cycle spec for non-simple cycles [OFN-11613]

35169f6

wandji20 force-pushed the wb-OFN-11613 branch from 6feab1b to 35169f6 Compare August 12, 2024 22:16

wandji20 requested a review from dacook August 12, 2024 22:32

dacook approved these changes Aug 12, 2024

View reviewed changes

drummer83 self-assigned this Aug 13, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Aug 13, 2024

drummer83 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Aug 13, 2024

drummer83 removed their assignment Aug 13, 2024

drummer83 merged commit 503148b into openfoodfoundation:master Aug 14, 2024
54 checks passed

This was referenced Aug 14, 2024

[Admin] [Order cycles] Cancel button of popup should redirect to OC list #12774

Closed

[Admin] [Order Cycles] Add warning popup to order cycle list as well #12775

Closed

drummer83 removed the feedback-needed label Aug 14, 2024

dacook changed the title ~~Add warning modal to order cycle with attached schedule general setting form [OFN-11613]~~ Add warning modal when changing dates for order cycle with linked orders [OFN-11613] Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653

Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653

wandji20 commented Jul 9, 2024

RachL commented Jul 19, 2024

rioug left a comment

rioug Jul 22, 2024

rioug Jul 22, 2024

rioug Jul 22, 2024

rioug Jul 22, 2024

dacook Jul 22, 2024

dacook left a comment

dacook Jul 22, 2024

dacook Jul 22, 2024

wandji20 commented Jul 24, 2024

dacook commented Jul 25, 2024

rioug left a comment

dacook left a comment

dacook Jul 29, 2024

dacook Jul 29, 2024

wandji20 Jul 29, 2024 •

edited

Loading

dacook Jul 29, 2024

wandji20 Jul 29, 2024

dacook left a comment

dacook Aug 12, 2024

drummer83 commented Aug 13, 2024

wandji20 commented Aug 13, 2024

drummer83 commented Aug 14, 2024

RachL commented Aug 14, 2024

drummer83 commented Aug 14, 2024

drummer83 commented Aug 14, 2024

		const dirty = this.inputTargets.some(ele =>
		new Date(this.initValsValue[`${ele.name}`]).getTime() !== new Date(ele.value).getTime());

Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653

Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653

Conversation

wandji20 commented Jul 9, 2024

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

RachL commented Jul 19, 2024

rioug 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

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

wandji20 commented Jul 24, 2024

dacook commented Jul 25, 2024

rioug 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

wandji20 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

drummer83 commented Aug 13, 2024

Test cases

Notes

Conclusion

wandji20 commented Aug 13, 2024

drummer83 commented Aug 14, 2024

RachL commented Aug 14, 2024

drummer83 commented Aug 14, 2024

drummer83 commented Aug 14, 2024

wandji20 Jul 29, 2024 •

edited

Loading