Skip to content

Conversation

@allendav
Copy link
Contributor

@allendav allendav commented Sep 16, 2021

Fixes #4922

Changes:

  • Update status remains on screen the entire time, showing whether it is checking for updates, has an update, or if the reader is already running the latest software
  • Update button is disabled unless an update is available, in which case it is enabled and primary
  • Disconnect button is disabled while checking for whether an update is available. If the check completes and no update is available, it is made primary

@adamzelinski - I've changed the info image to an activity spinner while we are checking for updates. I'll posted that movie here below. Please let me know if there other design changes you'd expect (see movies below)

@koke - I've based this off of issue/4990-display-version in order to pick up the changes there - once that is reviewed this can be rebased and merged to develop

Movie (with spinner added)

RPReplay_Final1631834941.MP4

Update release notes:

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

@allendav allendav added the feature: mobile payments Related to mobile payments / card present payments / Woo Payments. label Sep 16, 2021
@allendav allendav added this to the 7.6 milestone Sep 16, 2021
@allendav allendav requested a review from a team September 16, 2021 21:54
@allendav allendav changed the base branch from develop to issue/4990-display-version September 16, 2021 21:56
@allendav allendav requested a review from koke September 16, 2021 23:32
@allendav allendav marked this pull request as ready for review September 16, 2021 23:32
@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.

@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

Base automatically changed from issue/4990-display-version to develop September 17, 2021 18:16
@allendav
Copy link
Contributor Author

allendav commented Sep 17, 2021

Hey @joshheald - I was looking at the dark mode comment you posted here and I cannot reproduce it with this branch - can you give it a spin? This is what I see:

@joshheald
Copy link
Contributor

joshheald commented Sep 20, 2021

I cannot reproduce it with this branch - can you give it a spin?

Yep, looks good to me too @allendav

iPhone iPad
Screenshot 2021-09-20 at 08 54 42 Screenshot 2021-09-20 at 09 00 46

Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

I think this will work in most cases, but the state would be better kept on the service and not the view model. For instance, when you visit the settings page, it will start checking for an update. If you quickly navigate out of that view and go back in:

  • The view triggers a second check for updates while the first one is still in progress
  • The second check fails immediately because the SDK is busy with the first one
  • It fails silently as if there was no update found, and the disconnect button becomes enabled
  • If you tap to disconnect, the first check is still running, so the SDK is busy and it fails silently

I think the PR is good, but commenting more generally on the feature, I feel like we're giving too much weight to something that, while important, is quite infrequent. Every time you load that screen or connect to a reader, a new check is launched with prevents you from disconnecting until that's finished. The checkForUpdate operation is cancelable, so I think it might be better if we checked for updates as often as we wanted, but canceled that operation as soon as the user requested a different thing (disconnect, collect payment,...). I'd also hide the "Update Reader Software" button instead of having it disabled.

@allendav
Copy link
Contributor Author

I think this will work in most cases, but the state would be better kept on the service and not the view model.

I think so too.

If you quickly navigate out of that view and go back in:

Good point! I'll open a issue for moving this responsibility into the service

I feel like we're giving too much weight to something that, while important, is quite infrequent.

We are. The service could keep track of update checks and return a cached value if the last check was say within a day, with a pull to refresh allowing force refresh.

Another good idea for moving this to the service layer especially with SDK 2 changes

#5024

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

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Mobile Payments] Disconnect Reader button fails silently

4 participants