Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reset method to MatomoTracker #418

Closed
wants to merge 1 commit into from

Conversation

t-unit
Copy link

@t-unit t-unit commented Aug 29, 2022

Implements #417

Copy link
Member

@brototyp brototyp left a comment

Choose a reason for hiding this comment

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

Lovely! Thanks a lot. Looks really good. I really like how you adapted to the coding style of this repo! 🙇

There are a few thoughts from my side. None of them are really critical but rather thoughts about how to implement.

@@ -99,6 +99,18 @@ internal struct MatomoUserDefaults {
userDefaults.set(newValue, forKey: MatomoUserDefaults.Key.lastOrder)
}
}

func reset() {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about the term reset here. I was wondering, since this is a wrapper of UserDefaults if we want to keep the name close to the names of UserDefaults. So something like removeAllObjects or so. What do you think?

Comment on lines +104 to +112
userDefaults.removeObject(forKey: Key.totalNumberOfVisits)
userDefaults.removeObject(forKey: Key.currentVisitTimestamp)
userDefaults.removeObject(forKey: Key.previousVistsTimestamp)
userDefaults.removeObject(forKey: Key.firstVistsTimestamp)
userDefaults.removeObject(forKey: Key.clientID)
userDefaults.removeObject(forKey: Key.forcedVisitorID)
userDefaults.removeObject(forKey: Key.visitorUserID)
userDefaults.removeObject(forKey: Key.optOut)
userDefaults.removeObject(forKey: Key.lastOrder)
Copy link
Member

Choose a reason for hiding this comment

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

I could see the risk, once we add a new property that is stored in the user defaults, that we forget to remove it here as well. What do you think about changing the internal struct Key to an enum Key: String, CaseIterable so we can iterate over all of them here.

Comment on lines +472 to +491
extension MatomoTracker {
/// Resets all session, visitor and campaign information.
///
/// Dispatches events before restting itself.
/// After calling this method this instance behaves like the app has been freshly installed.
public func reset() {
dispatch()

matomoUserDefaults.reset()

visitor = Visitor.current(in: matomoUserDefaults)
session = Session.current(in: matomoUserDefaults)
dimensions = []
customVariables = []
campaignName = nil
campaignKeyword = nil

startNewSession()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

With this function we do have the risk, that if we add any properties, we forget to reset them in here as well. Unfortunately I don't have a great idea how to prevent this. I still wanted to bring this up here in case you have an idea.

One option would certainly be to replace the instance of MatomoTracker. But since the SDK doesn't provide any instance, this would be left for the user and all we could offer in this function here would be to reset the persisted data. I don't think that's great.

///
/// Dispatches events before restting itself.
/// After calling this method this instance behaves like the app has been freshly installed.
public func reset() {
Copy link
Member

Choose a reason for hiding this comment

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

I think, first think in this function, we should ensure we are running on the main thread.

@brototyp brototyp mentioned this pull request Jul 1, 2023
@brototyp
Copy link
Member

brototyp commented Jul 1, 2023

This PR is replaced by #434

@brototyp brototyp closed this Jul 1, 2023
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