-
-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warning modal when changing dates for order cycle with linked orders [OFN-11613] #12653
Conversation
84d0f65
to
ddc0766
Compare
@wandji20 same here, it looks like the PR needs a rebase |
05935ae
to
e7c7b54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
if(!this.hasScheduleValue) return; | ||
|
||
// Check that datetime input value has a change | ||
const dirty = this.inputTargets.some(ele => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about some()
}); | ||
} | ||
|
||
updateCallback(data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
this.modalConfirmTargets.forEach(ele => { | ||
if (e.target.getAttribute('data-target') === ele.getAttribute('data-request')) { | ||
ele.style.display = 'unset'; | ||
} else { | ||
ele.style.display = 'none'; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :
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'} }
%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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
const dirty = this.inputTargets.some(ele => | ||
new Date(this.initValsValue[`${ele.name}`]).getTime() !== new Date(ele.value).getTime()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Then you won't need to add the initVals
values and the HTML can be cleaner.
%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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e106f40
to
30c3e8f
Compare
Hi |
Thanks sorry I wasn't able to review this week; we will review on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wandji20 ! The server side validation looks good, there just a couple of things to tweak and we should be good to go.
app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Wandji, I also have some comments, and request for some explanation
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to confess I still don't see what this is for.. could you please explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,14 @@ | |||
.modal-body.flex-column-gap-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this rule in use? .flex-column-gap-1
It looks similar to others so I suspect isn't necessary
.modal-body.flex-column-gap-1 | |
.modal-body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this one
I am using .flex-column.gap-1
so will remove .modal-body
d911d35
to
f55642e
Compare
…ribute change and trigger warning modal [OFN-11613]
e97fbf2
to
6feab1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🤷
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised to see that we don't have a helper method to handle this, it seems like a common action.
Hi @wandji20, Test cases
Notes
ConclusionEverything is working well! 💪 Thanks again! 🙏 |
Thanks for testing @drummer83 |
Let's see what @RachL 's opinion is on this. 🙏 |
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 :
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? |
I agree, the topic around the order state 'cart' is an edge case. No need for action here, I think. |
What? Why?
To enable a warning modal when editing an order cycle with a linked schedule, I added
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):
Add warning modal to order cycle with attached schedule general setting form
Dependencies
N/A
Documentation updates
N/A