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

Paid events #25

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Paid events #25

merged 7 commits into from
Sep 9, 2024

Conversation

bbijoch
Copy link
Contributor

@bbijoch bbijoch commented Sep 5, 2024

No description provided.

@@ -39,6 +39,7 @@ final class KeepAliveManager {
private var pauseTimeStart = [Date]()
private var pauseTimeEnd = [Date]()

private let timerQueue = DispatchQueue(label: "timerQueue", attributes: .concurrent)
Copy link
Contributor Author

@bbijoch bbijoch Sep 5, 2024

Choose a reason for hiding this comment

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

To jest fix na testy które na fastlanie nie przechodziły. Timery były uruchamiane z qos: .background. Teraz zrobiłem dedykowaną kolejkę .concurrent
U mnie lokalnie testy na fastlane też nie przechodziły, a teraz już przechodzą

reportEvent(event)
}

// swiftlint:disable function_parameter_count

Choose a reason for hiding this comment

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

to jest zaszłość z lokalnego testowania, czy ma zostać?

Copy link
Contributor

Choose a reason for hiding this comment

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

nie, to wyłączenie lintera na ten przypadek, moze zostac

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lint ma ustawioną regułę na maxymalną liczę parametrów 5 (w tym projekcie), a chciałem aby Android i iOS miały taki sam interface. No i trzeba zignorować tą regułę.

/// - contentMetadata: Content metadata
/// - temporaryUserId: Temporary user id
/// - realUserId: New user id
func reportMobileAppTemporaryUserIdReplacedEvent(contentMetadata: ContentMetadata?,
Copy link
Contributor

Choose a reason for hiding this comment

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

tu ta meta nie jest potrzebna, to sie nie dzieje w kontekście żadnym

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielcalka jeśli mamy to na androidzie to do wywalenia też

reportEvent(event)
}

/// Reports event of piano prediction of user likelihood to subscribe / cancel subscription
Copy link
Contributor

Choose a reason for hiding this comment

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

nie powiniśmy tu o Piano wspominać - ten moduł nic o tym nie wie

/// - contentMetadata: Content metadata
/// - supplierData: Data regarding the supplier of sales
/// - metricsData: Metric counter data
func reportShowMetricLimitEvent(contentMetadata: ContentMetadata?,
Copy link
Contributor

Choose a reason for hiding this comment

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

tu meta powinno być wymagane bo bez tego to zdarzenie nie ma sensu

reportEvent(event)
}

// swiftlint:disable function_parameter_count
Copy link
Contributor

Choose a reason for hiding this comment

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

nie, to wyłączenie lintera na ten przypadek, moze zostac

import Foundation

/// Subscription payment data
public struct SubscriptionPaymentData {
Copy link
Contributor

Choose a reason for hiding this comment

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

musisz dodać publiczny konstruktor

import Foundation

/// Metric counter data
public struct MetricsData {
Copy link
Contributor

Choose a reason for hiding this comment

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

konstruktor


import Foundation

public struct LikelihoodData: Encodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

konstruktor

case realUserId = "real_user_id"
}

let fakeUserId: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

to ma tylko sens jak są dwie wartości

Choose a reason for hiding this comment

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

@adamszeremeta ten obiekt wykorzystywany jest tez przy createPurchaseEvent, gdzie idzie tylko fake_user_id

Copy link
Contributor

Choose a reason for hiding this comment

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

ale tam ma Bernard to podane jawnie jako
temporaryUserId: String?

bo potem to nie wymusza w tym zdarzeniu z przepisaniem usera podania dwoch wartosci.. chyba ze przez konstruktor to wymusimy a zostanie do uzytku wewnetrznego opcjonalne to ok


import Foundation

enum PaidEventParameter: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

wybacz ale ja bym sie trzymał konwencji że jawnie przy budowaniu podajemy klucze
patrząc na to ja nie mam pojęcia co idzie do backendu, nie wiem jak to jest enkodowane

co ty na to by zrobić to tak jak do tej pory?

// RingPublishingTracking
//
// Created by Bernard Bijoch on 04/09/2024.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

nie wszedzie jest copyright, przejrzyj i dodaj w plikach

/// Payment method
let paymentMethod: PaymentMethod

public init(subscriptionBasePrice: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

dorzuć jeszcze komentarze z parametrami do tych konstruktorów i bedzie ok wszystko

@bbijoch bbijoch merged commit 741d6b3 into feature/paid Sep 9, 2024
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.

3 participants