Skip to content

logIn/logOut: sync attributes before aliasing#1716

Merged
NachoSoto merged 2 commits into
mainfrom
attributes-bug-3
Jun 22, 2022
Merged

logIn/logOut: sync attributes before aliasing#1716
NachoSoto merged 2 commits into
mainfrom
attributes-bug-3

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Jun 19, 2022

Copy link
Copy Markdown
Contributor

Fixes CSDK-75.

Depends on #1715.

@NachoSoto NachoSoto requested a review from a team June 19, 2022 19:08
Comment thread Sources/Identity/IdentityManager.swift Outdated

@NachoSoto NachoSoto Jun 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was not feeling inspired yesterday, this needs a better name I think. Any suggestions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would have made it even worse with AttributeSyncable so yeah, I've got nothing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's an old habit but with interfaces/protocols when it adds some ability, I always add able to the end.

Comment thread Sources/Identity/IdentityManager.swift Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This could use a better name too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's wrong with logOut? I think it's ok.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are 2 logOut methods now. The internal one performs the sync first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oooooo maybe rename internal to syncThenLogOut?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of that cause that's exposing an implementation detail in the name :/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm on the fence, though. If it's internal, the naming actually serves to remind us why we have it. If it were public API I'd definitely say you're 100% correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see what you're saying.
It does feel weird for Purchases.logIn to call IdentityManager.syncAttributesAndLogIn() or something like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, ultimately, I don't have a strong preference for non-public api naming here. What you have perform is totally workable IMO

@NachoSoto NachoSoto changed the title logIn/logOut: fix: sync attributes before aliasing logIn/logOut: sync attributes before aliasing Jun 21, 2022

@taquitos taquitos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Naming question, but otherwise looks ✅

Comment thread Sources/SubscriberAttributes/SubscriberAttributesManager.swift Outdated
Base automatically changed from attributes-bug-2 to main June 21, 2022 19:00
@NachoSoto NachoSoto merged commit d86ffe0 into main Jun 22, 2022
@NachoSoto NachoSoto deleted the attributes-bug-3 branch June 22, 2022 18:30
@NachoSoto NachoSoto mentioned this pull request Jun 30, 2022
NachoSoto added a commit that referenced this pull request Jul 4, 2022
### Changes:
* Replaced `CustomerInfo.nonSubscriptionTransactions` with a new non-`StoreTransaction` type (#1733) via NachoSoto (@NachoSoto)
* `Purchases.configure`: added overload taking a `Configuration.Builder` (#1720) via NachoSoto (@NachoSoto)
* Extract Attribution logic out of Purchases (#1693) via Joshua Liebowitz (@taquitos)
* Remove create alias (#1695) via Joshua Liebowitz (@taquitos)

All attribution APIs can now be accessed from `Purchases.shared.attribution`.

### Improvements:
* Improved purchasing logs, added promotional offer information (#1725) via NachoSoto (@NachoSoto)
* `PurchasesOrchestrator`: don't log attribute errors if there are none (#1742) via NachoSoto (@NachoSoto)
* `FatalErrorUtil`: don't override `fatalError` on release builds (#1736) via NachoSoto (@NachoSoto)
* `SKPaymentTransaction`: added more context to warnings about missing properties (#1731) via NachoSoto (@NachoSoto)
* New SwiftUI Purchase Tester example (#1722) via Josh Holtz (@joshdholtz)
* update docs for `showManageSubscriptions` (#1729) via aboedo (@aboedo)
* `PurchasesOrchestrator`: unify finish transactions between SK1 and SK2 (#1704) via NachoSoto (@NachoSoto)
* `SubscriberAttribute`: converted into `struct` (#1648) via NachoSoto (@NachoSoto)
* `CacheFetchPolicy.notStaleCachedOrFetched`: added warning to docstring (#1708) via NachoSoto (@NachoSoto)
* Clear cached offerings and products after Storefront changes (2/4) (#1583) via Juanpe Catalán (@Juanpe)
* `ROT13`: optimized initialization and removed magic numbers (#1702) via NachoSoto (@NachoSoto)

### Fixes:
* `logIn`/`logOut`: sync attributes before aliasing (#1716) via NachoSoto (@NachoSoto)
* `Purchases.customerInfo(fetchPolicy:)`: actually use `fetchPolicy` parameter (#1721) via NachoSoto (@NachoSoto)
* `PurchasesOrchestrator`: fix behavior dealing with `nil` `SKPaymentTransaction.productIdentifier` during purchase (#1680) via NachoSoto (@NachoSoto)
* `PurchasesOrchestrator.handlePurchasedTransaction`: always refresh receipt data (#1703) via NachoSoto (@NachoSoto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants