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

Feature/custom variables #205

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions PiwikTracker/CustomVariable.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

import Foundation


/// See Custom Variable documentation here: https://piwik.org/docs/custom-variables/
public struct CustomVariable {
/// The name of the variable.
let name: String

/// The value of the variable.
let value: String
}
6 changes: 4 additions & 2 deletions PiwikTracker/Event.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ public struct Event {
/// api-key: urlref
let referer: URL?
let screenResolution : CGSize = Device.makeCurrentDevice().screenSize

/// api-key: _cvar
//let customVariables: [CustomVariable]
let customVariables: [CustomVariable]

/// Event tracking
/// https://piwik.org/docs/event-tracking/
Expand All @@ -71,7 +72,7 @@ public struct Event {
}

extension Event {
public init(tracker: PiwikTracker, action: [String], url: URL? = nil, referer: URL? = nil, eventCategory: String? = nil, eventAction: String? = nil, eventName: String? = nil, eventValue: Float? = nil, customTrackingParameters: [String:String] = [:], dimensions: [CustomDimension] = []) {
public init(tracker: PiwikTracker, action: [String], url: URL? = nil, referer: URL? = nil, eventCategory: String? = nil, eventAction: String? = nil, eventName: String? = nil, eventValue: Float? = nil, customTrackingParameters: [String:String] = [:], dimensions: [CustomDimension] = [], variables: [CustomVariable] = []) {
self.siteId = tracker.siteId
self.uuid = NSUUID()
self.visitor = tracker.visitor
Expand All @@ -88,5 +89,6 @@ extension Event {
self.eventValue = eventValue
self.dimensions = tracker.dimensions + dimensions
self.customTrackingParameters = customTrackingParameters
self.customVariables = tracker.customVariables + variables
}
}
60 changes: 57 additions & 3 deletions PiwikTracker/PiwikTracker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ final public class PiwikTracker: NSObject {

/// Defines if the user opted out of tracking. When set to true, every event
/// will be discarded immediately. This property is persisted between app launches.
public var isOptedOut: Bool {
@objc public var isOptedOut: Bool {
get {
return PiwikUserDefaults.standard.optOut
}
Expand All @@ -36,6 +36,8 @@ final public class PiwikTracker: NSObject {

internal var dimensions: [CustomDimension] = []

@objc public var useDefaultCustomVariables: Bool = false
private lazy var cvars: [CustomVariable] = getDefaultCVars()
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the cvars variable name here to much. We usually don't use abbreviations. What do you think about using customVariables here? This then shows another minor issue. It is imho hard to distinguish (just by the variable names) between the cvars and the customVariables that you implemented as a slice from these here.

What do you think about removing the useDefaultCustomVariables completely and let the users of the SDK add the "default" variables themselves? Exposing a defaultVariables property on the CustomVariable for example could help here. This way, the code here in the SDK is much cleaner and it is more direct for the user.


/// This logger is used to perform logging of all sorts of piwik related information.
/// Per default it is a `DefaultLogger` with a `minLevel` of `LogLevel.warning`. You can
Expand Down Expand Up @@ -139,7 +141,7 @@ final public class PiwikTracker: NSObject {

// MARK: dispatch timer

public var dispatchInterval: TimeInterval = 30.0 {
@objc public var dispatchInterval: TimeInterval = 30.0 {
didSet {
startDispatchTimer()
}
Expand Down Expand Up @@ -281,18 +283,70 @@ extension PiwikTracker {
dimensions.append(dimension)
}

/// Set a permanent custom dimension by value and index.
///
/// This is a convenience alternative to set(dimension:) and calls the exact same functionality. Also, it is accessible from Objective-C.
///
/// - Parameter value: The value for the new Custom Dimension
/// - Parameter forIndex: The index of the new Custom Dimension
@objc public func setDimension(_ value: String, forIndex index: Int) {
set(dimension: CustomDimension( index: index, value: value ));
}

/// Removes a previously set custom dimension.
///
/// Use this method to remove a dimension that was set using the `set(value: String, forDimension index: Int)` method.
///
/// - Parameter index: The index of the dimension.
public func remove(dimensionAtIndex index: Int) {
@objc public func remove(dimensionAtIndex index: Int) {
dimensions = dimensions.filter({ dimension in
dimension.index != index
})
}
}


extension PiwikTracker {

internal func getDefaultCVars() -> [CustomVariable] {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about moving this to the CustomVariable struct? Something like this:

extension CustomVariable {
    static var defaultVariables: [CustomVariable] {
        // return ...
    }
}

let currentDevice = Device.makeCurrentDevice()
let app = Application.makeCurrentApplication()

return [
CustomVariable( name: "Platform", value: currentDevice.platform ),
CustomVariable( name: "OS version", value: currentDevice.osVersion ),
CustomVariable( name: "App version", value: app.bundleVersion ?? "unknown" )
]
}

/// - Returns: A view on the Custom Variables.
var customVariables: ArraySlice<CustomVariable> {
let startIndex = useDefaultCustomVariables ? 0 : 3
return cvars[startIndex...]
}

/// Adds a new Custom Variable.
///
/// - Parameter name: The name of the new Custom Variable
/// - Parameter value: The value of the new Custom Variable
/// - Returns: The index of the new parameter. Note that indices start at 3 to accommodate the default Custom Variables. Also note that when default Custom Variables are turned off, the returned index is offset by 3 to what is actually sent to the Piwik API.
@objc @discardableResult public func addCustomVariable(_ name: String, value: String) -> Int {
cvars.append(CustomVariable(name: name, value: value))
return cvars.count
}

/// Remove a previously set Custom Variable. Note that the default Custom Variables cannot be removed.
///
/// Note that, as with any array, the index of any succeeding Custom Variables is decremented by 1.
///
/// - Parameter index: The index that was previously returned by addCustomVariable().
@objc public func removeCustomVariableAtIndex(_ index: Int) {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about referencing the variables by name, not by index? In this case I think the CustomVariable struct should bring the index to be referenced in the API.

@objc public func removeCustomVariable(withName name: String) {
    cvars = cvars.filter{$0.name != name }
}

assert(index >= 3 && index < cvars.count, "Index out of bounds")
cvars.remove(at: index)
}

}

// Objective-c compatibility extension
extension PiwikTracker {

Expand Down
18 changes: 14 additions & 4 deletions PiwikTracker/URLSessionDispatcher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,15 @@ final class URLSessionDispatcher: Dispatcher {
}

fileprivate extension Event {

private func cvarParameterValue() -> String {
var cvars: Array<String> = customVariables.enumerated().map { "\"\($0.offset + 1)\":[\"\($0.element.name)\",\"\($0.element.value)\"]" }
return "{\(cvars.joined(separator: ","))}"
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 this code is a bit to complex to parse. Do you think it is possible to optimize it?

}

var queryItems: [URLQueryItem] {
get {
let items = [
var items = [
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep this as a let and the join al the different query items together in the end?

URLQueryItem(name: "idsite", value: siteId),
URLQueryItem(name: "rec", value: "1"),
// Visitor
Expand Down Expand Up @@ -128,9 +134,13 @@ fileprivate extension Event {

].filter { $0.value != nil }

let dimensionItems = dimensions.map { URLQueryItem(name: "dimension\($0.index)", value: $0.value) }
let customItems = customTrackingParameters.map { return URLQueryItem(name: $0.key, value: $0.value) }
return items + dimensionItems + customItems
items += dimensions.map { URLQueryItem(name: "dimension\($0.index)", value: $0.value) }
items += customTrackingParameters.map { return URLQueryItem(name: $0.key, value: $0.value) }
if customVariables.count > 0 {
items.append( URLQueryItem(name: "_cvar", value: cvarParameterValue()) )
}

return items
}
}
}
Expand Down