Skip to content

Conversation

@Ecarrion
Copy link
Contributor

part of #5483

Why

Previous PR #5502 took care of rendering proper taxes on the summary screen, this PR takes care of updating the order remotely with the user selection.

PS: This PR just updates the order, does not navigate to any new screen, and does not handle errors yet.

How

  • Updates OrderFeeLine to encode its ID. In order to be able to update the simple payment fee with taxable or not taxable status

  • Updates OrdersRemote to support encoding fee lines when updating an order

  • Updates OrderStore with an action to update a simple payments order.

  • Update SummaryViewModel:

    • Receive via DI all the necessary fields in order to update an order
    • Call the update order action
    • Provide a loadingIndicator property
  • Update SummaryView to consume the new values from its VM

Demo

update-order.mov

Testing

Prerequisites

  • Make sure you are using an IPP eligible store
  • Make sure you have a store with taxes set for your store location

Steps

  • Start the simple payments flow
  • Enter an amount & tap next
  • Enter email, modify taxes, edit note
  • Tap "Take Payment Button"
  • See a loading spinner on the button
  • After it finishes, dismiss the flow and check the order detail to corroborate the changes have been correctly made

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@Ecarrion Ecarrion added the feature: order creation All tasks related to creating an order label Nov 25, 2021
@Ecarrion Ecarrion added this to the 8.1 milestone Nov 25, 2021
@Ecarrion Ecarrion self-assigned this Nov 25, 2021
@Ecarrion Ecarrion force-pushed the issue/5483-update-order branch from 770d7cc to 791755b Compare November 25, 2021 17:19
@peril-woocommerce
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-woocommerce
Copy link

You can trigger an installable build for these changes by visiting CircleCI here.

@Ecarrion Ecarrion mentioned this pull request Nov 25, 2021
1 task
@Ecarrion Ecarrion removed their assignment Nov 26, 2021
@rachelmcr rachelmcr self-assigned this Nov 26, 2021
Copy link
Contributor

@rachelmcr rachelmcr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The changes are clear and test well. 👍


Not blocking or specific to this PR, but an observation from my testing:

At first I thought perhaps the note wasn't getting updated when the order was updated. Because it's called "Order Note" in the Simple Payments UI, I looked in the "Order notes" section in the order detail (check both in the app and in wp-admin) expecting to see the note there. It wasn't until I looked again at the code that I realized this note was going into the customer notes section.

I know this behavior has been discussed before, but I'm not sure if it's something we'd reconsider. Aside from the naming confusion, I also expect that customer-provided notes are only things the customer needs to communicate to the merchant (e.g. shipping/delivery instructions) while the order notes a merchant might enter while creating an order would be either for internal purposes (e.g. noting what the payment was for) or for communicating something to the customer with the order confirmation.

Screenshots:

Simple payments - Order Note field Order details in app Order details in wp-admin
Screen Shot 2021-11-26 at 3 32 00 PM Screen Shot 2021-11-26 at 3 33 45 PM Screen Shot 2021-11-26 at 3 32 38 PM

@Ecarrion
Copy link
Contributor Author

Hi @rachelmcr Thanks for the review. I agree that the naming is a bit confusing 🤔

We have decided to use the "customer provided note" for some reasons:

  • It gets rendered in the receipt
  • Information does not get lost within other notes as there could be a lot of auto-generated notes when making payments and updating statuses.
  • Order note API requires a separate API call.

What are your thoughts on this @adamzelinski?

@Ecarrion Ecarrion merged commit 174524c into develop Nov 26, 2021
@Ecarrion Ecarrion deleted the issue/5483-update-order branch November 26, 2021 16:27
@ghost
Copy link

ghost commented Nov 29, 2021

Hey, @Ecarrion @rachelmcr I agree and I'm happy to use 'Customer Provided Note' too. Thanks!

@Ecarrion
Copy link
Contributor Author

Ecarrion commented Nov 29, 2021

Cool, I will update the name to "Customer Provided Note" to avoid confusion.
cc @nbradbury

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: order creation All tasks related to creating an order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants