Skip to content

/ brave-ios

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

No Bug: Use Calendar's api to calculate DAU pings. #796

Merged
merged 3 commits into from Jan 25, 2019
Merged

No Bug: Use Calendar's api to calculate DAU pings. #796

merged 3 commits into from Jan 25, 2019

Conversation

@iccub
Copy link
Contributor

iccub commented Jan 25, 2019 •
edited by jhreis

Credit for the Calendar based solution goes to @kylehickinson

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adaquate test plan exists for QA to validate (if applicable)
@iccub iccub requested review from kylehickinson and jhreis Jan 25, 2019
@iccub iccub changed the base branch from development to master Jan 25, 2019
@kylehickinson
kylehickinson requested changes Jan 25, 2019
View changes
BraveShared/Analytics/DAU.swift Outdated Show resolved Hide resolved
BraveShared/Analytics/DAU.swift Outdated
let monthly = pings.first(where: { $0 == .monthly })

// No changes, no ping
if daily == nil && weekly == nil && monthly == nil {

This comment has been minimized.

Sign in to view
Copy link
@kylehickinson

kylehickinson Jan 25, 2019

Contributor

Why not just check if !pings.isEmpty?
@jhreis
jhreis suggested changes Jan 25, 2019
View changes
BraveShared/Analytics/DAU.swift Outdated
}

private func getPings(forDate date: Date, lastPingDate: Date) -> [PingType] {
let calendar = DAU.calendar as Calendar

This comment has been minimized.

Sign in to view
Copy link
@jhreis

jhreis Jan 25, 2019

Contributor

Does calendar still need to return an NSCalendar?
BraveShared/Analytics/DAU.swift Outdated
let monthly = pings.first(where: { $0 == .monthly })

// No changes, no ping
if daily == nil && weekly == nil && monthly == nil {

This comment has been minimized.

Sign in to view
Copy link
@jhreis

jhreis Jan 25, 2019

Contributor

Can just check the length of pings

if pings.count == 0 {}
BraveShared/Analytics/DAU.swift Outdated
return nil
}

return dauParams(daily, weekly, monthly)
return dauParams(daily != nil, weekly != nil, monthly != nil)

This comment has been minimized.

Sign in to view
Copy link
@jhreis

jhreis Jan 25, 2019

Contributor

I think I'd have dauParams just take an array too, honestly, than no need for all the logic above.
BraveShared/Analytics/DAU.swift Outdated
log.debug("Dau stat params, daily: \(daily), weekly: \(weekly), monthly:\(monthly), lastPingDate: \(lastPingDate)")
if !daily && !weekly && !monthly {
// No changes, no ping
let daily = pings.first(where: { $0 == .daily })

This comment has been minimized.

Sign in to view
Copy link
@jhreis

jhreis Jan 25, 2019

Contributor

let daily = pings.contains(.daily)
@iccub iccub force-pushed the iccub:dau-new-algo branch 2 times, most recently from c8b059b to d01ee56 Jan 25, 2019
@kylehickinson
kylehickinson reviewed Jan 25, 2019
View changes
BraveShared/Analytics/DAU.swift Outdated
func eraDayOrdinal(_ date: Date) -> Int? {
return calendar.ordinality(of: .day, in: .era, for: date)
}
func nextDateComponent(_ components: DateComponents) -> Date? {

This comment has been minimized.

Sign in to view
Copy link
@kylehickinson

kylehickinson Jan 25, 2019

Contributor

OCD nit:

Suggested change
func nextDateComponent(_ components: DateComponents) -> Date? {
func nextDate(matching components: DateComponents) -> Date? {
@kylehickinson
kylehickinson approved these changes Jan 25, 2019
View changes
Copy link
Contributor

kylehickinson left a comment

You can fix my OCD nit or not doesn't matter 😄
@iccub iccub force-pushed the iccub:dau-new-algo branch from acbe6b6 to 68758fe Jan 25, 2019
@kylehickinson
kylehickinson reviewed Jan 25, 2019
View changes
BraveShared/Analytics/DAU.swift Outdated
guard let mYear = mondayComponents.year, let mMonth = mondayComponents.month, let mDay = mondayComponents.day else {
log.error("First monday of the week components are nil")
return ""
if considerToday && calendar.component(.weekday, from: self) == weekday.rawValue {

This comment has been minimized.

Sign in to view
Copy link
@kylehickinson

kylehickinson Jan 25, 2019

Contributor

Considering this is an extension on Date which can be any date, couldn't calendar.component(.weekday, from: self) be any Monday and not be today?

This comment has been minimized.

Sign in to view
Copy link
@iccub

iccub Jan 25, 2019

Author Contributor

I'm not sure if I understand you.
This can be any weekday, depending on what date we are going to pass and against which weekday we are comparing.
Should I make it more specific and hide behind private accessor?

This comment has been minimized.

Sign in to view
Copy link
@kylehickinson

kylehickinson Jan 25, 2019 •
edited

Contributor

Basically Im confused because you're saying "get me the next weekday and consider today" but there's no concept of what day "today" is -- is it just the receiver date? I.e.

If the receiver date/self is Friday the 18th but Today is Friday the 25th and you say "give me the next friday" considering today, it'll just give you Friday the 18th.

So to clarify it you would either have to change it to explicitly give what "Today" is, or instead just change the naming, so that instead of "Consider Today" it's "Consider this (self) date's weekday"

This comment has been minimized.

Sign in to view
Copy link
@kylehickinson

kylehickinson Jan 25, 2019

Contributor

So yeah I don't think based on usage this is a problem because its always called on today, was just a bit confusing to understand when not paying attention to the call site/only paying attention to the fact that this is a method on any Date

This comment has been minimized.

Sign in to view
Copy link
@iccub

iccub Jan 25, 2019

Author Contributor

I think renaming to considerSelf would make sense
@iccub iccub force-pushed the iccub:dau-new-algo branch from 68758fe to 39dcb5f Jan 25, 2019
@jhreis
jhreis approved these changes Jan 25, 2019
View changes
iccub added 2 commits Jan 25, 2019
@iccub
No Bug: Use Calendar's api to calculate DAU pings.
0f1d20e
@iccub
No Bug: Use Calendar api for getting Monday of a week.
5d7c2fe
@iccub iccub force-pushed the iccub:dau-new-algo branch from 39dcb5f to 5d7c2fe Jan 25, 2019
@jhreis
jhreis reviewed Jan 25, 2019
View changes
BraveShared/Analytics/DAU.swift Outdated
let daily = monthly || lastPingDay != currentDay
let daily = pings.contains(.daily)
let weekly = pings.contains(.weekly)
let monthly = pings.contains(.monthly)

This comment has been minimized.

Sign in to view
Copy link
@jhreis

jhreis Jan 25, 2019

Contributor

If putting into variables, place them right before their usage down in dauPings
@iccub
sss
d09d639
@jhreis jhreis merged commit c85ca28 into brave:master Jan 25, 2019
jhreis added a commit that referenced this pull request Jan 28, 2019
@iccub @jhreis
No Bug: Use Calendar's api to calculate DAU pings. (#796)
Loading status checks…
0945891
@iccub iccub mentioned this pull request Nov 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylehickinson kylehickinson

@jhreis jhreis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@iccub @kylehickinson @jhreis
You can’t perform that action at this time.