woocommerce-ios icon indicating copy to clipboard operation
woocommerce-ios copied to clipboard

Remove orders/totals request from notifications manager

Open ealeksandrov opened this issue 3 years ago • 1 comments

Follow-up to: #7735

Description

This PR updates PushNotificationsManager to prevent triggering data reload notifications on each "app badge clear" action. Without it order list will trigger orders/totals requests on each viewWillAppear.

Same change was already implemented in https://github.com/woocommerce/woocommerce-ios/pull/7459, but Application Icon badge handling accidentally removed with it, so this PR now decouples one from another.

Note: Application icon badge != Orders tab badge. They have different logic and values (app icon can only be 1 or nil)

Testing instructions

  1. Allow notifications to display a badge on app icon
  2. Place an order to trigger app icon badge update (always "1" for any number of orders)
  3. Go to the orders tab and confirm that application badge is cleared, but orders/totals API request is not triggered.

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

ealeksandrov avatar Sep 16 '22 13:09 ealeksandrov

You can test the changes from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr7740-28627c1 on your iPhone
If you need access to App Center, please ask a maintainer to add you.

wpmobilebot avatar Sep 16 '22 13:09 wpmobilebot

Hey @ealeksandrov, thanks for tackling the https://github.com/woocommerce/woocommerce-ios/pull/7735 fix and adjusting the solution to avoid the problem again! Much appreciated, and sorry for introducing that bug 😅. Just one question to acquire some additional context: I noticed this PR hanging while I was AFK. Is the code here OK for review, or it's waiting for something?

ThomazFB avatar Oct 11 '22 17:10 ThomazFB

@ThomazFB no worries, I'm pretty sure it's not a final performance improvement that we'll find! PR is ready for review, it wasn't urgent to prioritize earlier and you should have the best context awareness around it 🙂

ealeksandrov avatar Oct 11 '22 18:10 ealeksandrov