-
-
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
Replace cable_ready responses with turbo_stream OFN-12794 #12896
base: master
Are you sure you want to change the base?
Conversation
c1080b7
to
73011b0
Compare
597a989
to
779e899
Compare
@@ -951,10 +951,12 @@ def new_order_with_distribution(distributor, order_cycle) | |||
order.finalize! # ensure order has a payment to capture | |||
order.payments << create(:check_payment, order:, amount: order.total) | |||
order.payments.first.capture! | |||
login_as_admin |
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 could not figure out why the spec @enterprise_user
had no permission to perform order ship action.
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.
Great work, that's awesome @wandji20 ! I just have a couple of questions, see my comments.
visit spree.edit_admin_order_path(order) | ||
end | ||
|
||
it "ships the order and shipment email is sent" do | ||
login_as_admin |
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.
Why is this needed ? it should already be logged as an admin because it's done in the before
block.
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.
These specs are in the context of an enterprise manager and we have an enterprise manager signed in this block
before(:each) do
@enterprise_user = create(:user)
@enterprise_user.enterprise_roles.build(enterprise: supplier1).save
@enterprise_user.enterprise_roles.build(enterprise: coordinator1).save
@enterprise_user.enterprise_roles.build(enterprise: distributor1).save
login_as @enterprise_user
end
```
I had some trouble figuring out why the permission failed for the signed in enterprise user when trying to ship an order.
So I was hoping to get help during review.
@@ -972,6 +974,7 @@ def new_order_with_distribution(distributor, order_cycle) | |||
end | |||
|
|||
it "ships the order without sending email" do | |||
login_as_admin |
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.
Same as above
= render ModalComponent.new(id: "resend_confirmation", modal_class: 'tiny', close_button: false) do | ||
= form_with(url: resend_confirmation_emails_admin_orders_path, method: :post, data: { turbo: true, controller: 'bulk-actions' }) do | ||
.margin-bottom-30 | ||
= t('.resend_confirmation_confirm_html') | ||
%p.modal-actions.justify-space-around | ||
%button.button.secondary{ type: "button", 'data-action': 'click->modal#close' } | ||
= t('js.admin.modals.cancel') | ||
%button.button.primary{ type: 'submit' } | ||
= t('js.admin.modals.confirm') | ||
|
||
= render ModalComponent.new(id: "send_invoice", modal_class: 'tiny', close_button: false) do | ||
= form_with(url: send_invoices_admin_orders_path, method: :post, data: { turbo: true, controller: 'bulk-actions' }) do | ||
.margin-bottom-30 | ||
= t('.send_invoice_confirm_html') | ||
%p.modal-actions.justify-space-around | ||
%button.button.secondary{ type: "button", 'data-action': 'click->modal#close' } | ||
= t('js.admin.modals.cancel') | ||
%button.button.primary{ type: 'submit' } | ||
= t('js.admin.modals.confirm') | ||
|
||
|
||
= render ModalComponent.new(id: "cancel_orders", modal_class: 'tiny', close_button: false) do | ||
= form_with(url: cancel_orders_admin_orders_path, method: :post, data: { turbo: true, controller: 'bulk-actions' }) do | ||
.margin-bottom-30 | ||
= t("js.admin.orders.cancel_the_order_html") | ||
.margin-bottom-30 | ||
= render partial: "spree/admin/orders/messages/cancel_orders" | ||
|
||
%p.modal-actions.justify-space-around | ||
%button.button.secondary{ type: "button", 'data-action': 'click->modal#close' } | ||
= t('js.admin.modals.cancel') | ||
%button.button.primary{ type: 'submit' } | ||
= t('js.admin.modals.confirm') |
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.
Any reasons why you are not using ConfirmModalComponent
here ?
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 did this simply because the confirm modal did not support handling forms without refactoring.
I will refactor to remove these duplications.
fetch(e.target.getAttribute('data-url'), { | ||
method: "POST", | ||
headers: { | ||
Accept: "text/vnd.turbo-stream.html", | ||
"Content-Type": "application/json", | ||
"X-CSRF-Token": csrfToken | ||
}, | ||
body: JSON.stringify(data), | ||
}) | ||
.then((response) => response.text()) | ||
.then((html) => { | ||
Turbo.renderStreamMessage(html); | ||
}) | ||
.catch((error) => console.error(error)); |
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.
Maybe I am missing something here, but it feels like we are replicating something that Turbo
should be doing for us ? Is there a way to refactor this so we don't have to manually use fetch
?
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.
My main reason was to be able to send data (bulk_ids) when printing invoices.
Another option I could think of was to use a button_to
tag for the Print invoices
bulk action and update the builk_ids input in the underlying generated form as I have done for the bulk_actions with a confirmation modal.
…ert to resend email confirmation with turbo_stream
…ns with turbo_stream
9f153e6
to
74ebcb2
Compare
74ebcb2
to
ad13f2e
Compare
What? Why?
Closes Replace mrujs with turbo #12794
Since Turbo already covers everything cable_ready does, we can safely move from cable_ready and mrujs to turbo.
What should we test?
The overall functionality of the app should be retained.
Release notes
Replace mrujs with turbo
Changelog Category (reviewers may add a label for the release notes):
Replace mrujs with turbo
Dependencies
N/A
Documentation updates
N/A