Notifications: Adds updateReadStatus to NotificationStore #405
Conversation
| onCompletion(nil) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nipticky: I forgot to mention something!!! (about that!!!)
In WPiOS we do a bit of a special treatment here. The scenario that prompted this was (afaik somehow) like this:
- Slow connection
- Notifications list gets onscreen
- User pushes the Details for "Notification A"
- Back to the Notes List
- [Time Elapsed]
- "Mark as Read" result comes back
At that point you'd see, out of the blue, the notification getting marked as read. What we do, to attempt to improve the UX, is to optimistically update the Stored Notification.
That way, when you hit (4), the note is "marked as read" locally.
If the remote call fails, we simply invalidate the cache (just set an invalid Note.hash value). This way, next time we sync, the "Optimistic OP" would be reverted.
No need to patch this here, right now, we can get back to it down the road, if needed.
Just wanted to mention it, because it's one of those... tiny annoyances i've dealt with 😄
There was a problem hiding this comment.
Thanks @jleandroperez I created #406 to capture that ☝️
… issue/19-notifications-read * 'develop' of github.com:woocommerce/woocommerce-ios: Updates Project Adding DataModel Mark 6
Another
NotificationStore&NotificationActionPR that wraps the Notifications "update read status" logic in Yosemite (including unit tests).Note that leaving the derived MOC hanging around in the static
NotificationStorevar was causing unit tests to randomly fail (and it makes me wonder if would also cause unforeseen issues within the app itself). So to remedy this, I added aNotificationStore.resetSharedDerivedStorage()func that will nil it out in-between tests as well as in the final closure onNotificationStore.synchronizeNotifications().Partially address #19
Testing
Make sure the logic is sound and the unit tests all pass!
@jleandroperez would you mind checking out this?