[MOB-1960] Update Referrals refresh logic #573
Conversation
d4r1091
changed the title
| @@ -508,7 +508,7 @@ final class MultiplyImpact: UIViewController, NotificationThemeable { | |||
| private func updateInviteLink() { | |||
| guard let inviteLink = inviteLink else { | |||
| copyLink?.text = "-" | |||
| referrals.refresh(createCode: true) {[weak self] error in | |||
| referrals.refresh(force: true, createCode: true) {[weak self] error in | |||
There was a problem hiding this comment.
At the moment, I see the only thing we do after the refresh is call self?.updateInviteLink() again so that copyLink?.text = inviteLink can be set. Since we are now refreshing the referrals and potentially get new claims, we should also refresh the counter when there is an update. I would even suggest properly subscribing to referrals since it is a Publisher and does already have a logic of sending whenever the count updates.
Also this might be a bit conflicting with the changes made on #568 since there a new component for the referral counter is used that would need to be updated 🤔 I guess it all depends on which is merged first to see how it is handled.
There was a problem hiding this comment.
As I'll be planning to move the refresh logic out of the updateInviteLink() we can also double-check whether this is still needed.
|
Reviewed logic to extract the Referrals' refresh while keeping the higher level logic as similar as possible. |
| private func refreshReferrals() { | ||
| referrals.refresh(force: true, createCode: true) { [weak self] error in | ||
| guard error == nil else { | ||
| self?.showObtainingCode(error!) |
There was a problem hiding this comment.
We also have showReferralError which might also be relevant in here.
Probably showReferralError and showObtainingCode can be merged now since the only difference is the retrying logic, which I guess can be the same 🤔
There was a problem hiding this comment.
The previous implementation was only referring to the showObtainingCode(:) error.
We have only separated the updateInviteLinks() from the refresh which is called at the same entry point now (viewWillAppear()).
To minimize in-scope changes only, I'd like to leave this small refactor off these changes for now.
There was a problem hiding this comment.
The problem I see with not changing it is that now with the new implementation we might reach an error on a different request and by showing this message from showObtainingCode(:) the retry attempt would be only calling self?.updateInviteLink() which does not actually retry
There was a problem hiding this comment.
You also need to fetch the latest ios-core version so that the new increased cooldown takes effect, right? Or were you planning on doing it separately?
There was a problem hiding this comment.
Or were you planning on doing it separately?
Planned to update only after receiving a ✅ on these requested changes logic wise
| } | ||
| return | ||
| self?.updateInviteLink() |
There was a problem hiding this comment.
Also believe you need to update the UI for the counter and not only the invite link after the refresh happens. If you update from main with the changes from #568, this should be only updating referralImpactRowView.info.
d4r1091
force-pushed
the
MOB-1960_referrals_refresh_review
branch
from
878c9df to
c1f8102
Compare
MOB-1960
Context
Due to a regression found on G Changes, we take the occasion to review the Referrals update logic.
Approach
HomepageViewModelProtocol'srefreshData(:)to conform to other cells and delegate its ViewModel to refresh its own data. As the HomePageViewController sits within theBrowserViewControllerdomain,viewWillAppearwon't respond every time theHomePageViewControlleris shown. TheBrowserViewControllercreates the object only once and then orchestrates the transitions between views accordingly.referrals.refreshspecifying theforce:trueparameter within theupdateInviteLink()function called inviewWillAppear