woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
258 stars 108 forks source link

[Shipping Label] Add condition checks before showing "Email" and "Message" actions #8982

Closed reginabally closed 1 year ago

reginabally commented 1 year ago

Closes: #7983

Description

If the customer address can't be verified during shipping label creation, the app will propose to contact the customer via email / message / call. Tapping on the "Email" or "Message" action does nothing if the device doesn't have Mail or Message apps configured.

This PR adds additional checks before displaying the "Email" and "Message" actions after user taps the "Contact Customer" button on the top banner.

Testing instructions

Pre-requisite:

  1. The test store must have WooCommerce Shipping & Tax plugin installed and activated.
  2. Uninstall the Mail app on the device
  3. Turn on the Airplane mode on the device, enable WiFi, and disable iMessage. This should prevent the app from being able to send out SMS.

Testing steps:

  1. Create an order and collect the payment
  2. Tap Create Shipping Label and fill in the Ship from data (first screen)
  3. Fill the data for the Ship to screen with a semantically valid but non-existing address and tap Done
  4. Once the We were unable to verify shipping address... alert is shown, tap the Contact Customer button
  5. Notice the "Email" and "Messages" options will not be shown.

Screenshots

https://user-images.githubusercontent.com/40906847/221175007-df521b19-48ce-492b-884b-7b70f8e424ed.mp4


reginabally commented 1 year ago

Hi @rachelmcr, Thanks in advance for reviewing this PR! I tested this change on Xcode Simulator, where Mail and Messages apps are not available. So I haven't been able to test whether the "Email" or "Message" option will be available when both apps work.

Also, I need some guidance to address another issue reported in #7983:

there's no animation when Contact Customer is tapped, same for Open Map

I can't seem to find a button in the app where we show animation when tapped. Can you please show me some sample screens and buttons where we show feedback when tapped? I'd like to standardize the approach for a better UX. Thanks!

wpmobilebot commented 1 year ago
You can test the changes from this Pull Request by: If you need access to App Center, please ask a maintainer to add you.
rachelmcr commented 1 year ago

Also, I need some guidance to address another issue reported in https://github.com/woocommerce/woocommerce-ios/issues/7983:

there's no animation when Contact Customer is tapped, same for Open Map

🤔 I'm not entirely sure what animation they were expecting there. It might be worth asking for clarification on the issue first, to make sure we know what was expected.

reginabally commented 1 year ago

Thanks for reviewing this PR, @rachelmcr!

That leads me to another question, though: Should we also add a check for the "Call" option, so it only appears if the device can make a call?

I've considered that, but if a device couldn't send emails and messages and was not even able to initiate a call, we would have nothing to show when the user taps the "Contact Customer" button.

The possible approaches I can think of when none of the three actions (Email, Message, Call) are available on the device are:

What do you think?

Edited to add a screenshot for the first approach:

rachelmcr commented 1 year ago

Good thought! I think either of those approaches would work. I'd lean slightly toward removing the button (since we know it won't do anything) but I don't feel super strongly about that. Since this is probably a rare scenario, it isn't worth agonizing over. I think go with whichever approach feels appropriate to you and straightforward to implement.

peril-woocommerce[bot] commented 1 year ago
Warnings
:warning: This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by :no_entry_sign: dangerJS

reginabally commented 1 year ago

Thanks, @rachelmcr! I committed the change with the first approach because it's more straightforward to me. Appreciate your time in reviewing the code again. 🙇🏻‍♀️

reginabally commented 1 year ago

Thanks again for reviewing, @rachelmcr!

It looks like a fake phone number snuck in to the PhoneHelper check you added here; let me know if I'm missing something about that.

It was initially supposed to be phone, but since it is optional, the canCallPhoneNumber condition check will fail if the customer didn't input a phone number, which defeats the purpose of checking the device's capability of opening the tel:// URL. That is the reason why I put a fake number there.

Perhaps I should do something like this?

if PhoneHelper.canCallPhoneNumber(phone: {
                if let phoneNumber = phone, phoneNumber.isNotEmpty {
                    return phoneNumber
                } else {
                    return "123456789" }
            }() ) == false
rachelmcr commented 1 year ago

the canCallPhoneNumber condition check will fail if the customer didn't input a phone number, which defeats the purpose of checking the device's capability of opening the tel:// URL.

Don't we want it to fail in that case? We don't want to show the "Call" option if there isn't a phone number to call.

reginabally commented 1 year ago

Don't we want it to fail in that case? We don't want to show the "Call" option if there isn't a phone number to call.

You're absolutely right! I was too focused on checking the device's capability but neglected that showing the "Call" option without a number to call is pointless.

I removed the fake number and replaced it with phone. Thanks again for your guidance and patience, @rachelmcr! 🙇🏻‍♀️

reginabally commented 1 year ago

Thank you very much for tirelessly showing me how to work on the issue, @rachelmcr!