Skip to content

Implement new Billing model #3147

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 8, 2021
Merged

Implement new Billing model #3147

merged 5 commits into from
Sep 8, 2021

Conversation

@S2Ler
Copy link
Contributor

@S2Ler S2Ler commented Jul 5, 2021 •
edited
Loading

  • Integrate with MapboxCommon
  • Address billing methods question in BillingHandler.getSessionToken()
  • Test that billing events work correctly
  • Find out what to do with all the edge cases

S2Ler
S2Ler commented Jul 5, 2021
View reviewed changes
@S2Ler S2Ler mentioned this pull request Jul 6, 2021
Closed
5 tasks
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 6 times, most recently from 550a436 to fd8b91b Compare July 15, 2021 12:33
@S2Ler S2Ler mentioned this pull request Jul 15, 2021
Closed
11 tasks
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 8 times, most recently from 0710773 to cdd04e6 Compare July 21, 2021 10:10
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 2 times, most recently from 1b7b80f to 8ba580f Compare August 2, 2021 12:21
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 5 times, most recently from 96f4b9e to aef27be Compare August 10, 2021 05:04
@S2Ler S2Ler self-assigned this Aug 10, 2021
@S2Ler S2Ler added this to the v2.0.0 (RC) milestone Aug 10, 2021
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 4 times, most recently from 25a2197 to f0c92cd Compare August 12, 2021 08:09
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 3 times, most recently from 14ce793 to 688e7e8 Compare August 20, 2021 06:50
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch from 804e95e to 6f938d4 Compare August 27, 2021 05:34
@S2Ler
Copy link
Contributor Author

S2Ler commented Aug 27, 2021 •
edited
Loading

Notable changes since last review:

  • Obtain access_token for BillingHandler from NavigationSettings.
  • Don't return sku_token for pause sessions.
  • sku_token now only uses information from BillingService in Common. That makes code more straightforward.
  • Starting CarPlay navigation pauses CarPlay PassiveLocationManager @MaximAlien please review this code.
  • New tests added.
  • Documentation updated.
  • Added logs for the case when billing session start fails.

@S2Ler S2Ler requested a review from 1ec5 August 31, 2021 08:14
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch from 7bce6b7 to 8b0c22d Compare August 31, 2021 09:17
1ec5
1ec5 reviewed Sep 1, 2021
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Kudos, this implementation is more sophisticated than anything I would’ve come up with in the time you put it together, and I think you’ve done a pretty good job of covering your bases with a variety of edge cases.

Sources/MapboxCoreNavigation/BillingHandler.swift Outdated
Comment on lines 1 to 2
Copy link
Contributor

@1ec5 1ec5 Sep 1, 2021

Choose a reason for hiding this comment

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

Link to the terms of service. Maybe CONTRIBUTING.md or LICENSE.md should mention that some files are subject to these restrictions?

Copy link
Contributor Author

@S2Ler S2Ler Sep 1, 2021

Choose a reason for hiding this comment

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

Thanks for suggestion it makes sense. Tracker for the url: https://github.com/mapbox/navigation-sdks/discussions/1120#discussioncomment-1264188

@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch 2 times, most recently from 882d7aa to 40c12a3 Compare September 3, 2021 13:19
@bamx23 bamx23 force-pushed the s2ler/billing-sessions branch from 40c12a3 to b8de6d4 Compare September 3, 2021 18:17
@S2Ler S2Ler requested a review from MaximAlien September 7, 2021 05:55
@S2Ler
Copy link
Contributor Author

S2Ler commented Sep 7, 2021

@MaximAlien please take a look on CarPlay related change.

@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch from b8de6d4 to 2be65fd Compare September 7, 2021 06:08
@S2Ler
Copy link
Contributor Author

S2Ler commented Sep 7, 2021

acc2fec60a1f016d5c39d443d8ad629cdbafe718 fixes #3329

cc @bamx23 @Udumft

@bamx23 bamx23 mentioned this pull request Sep 7, 2021
Closed
@S2Ler
Copy link
Contributor Author

S2Ler commented Sep 7, 2021

@1ec5 Please give a last read before we merge.

bamx23
bamx23 reviewed Sep 7, 2021
View reviewed changes
@Udumft Udumft linked an issue Sep 7, 2021 that may be closed by this pull request
[Bug]: PassiveLocationManager not stopped #3329
Closed
MaximAlien
MaximAlien reviewed Sep 8, 2021
View reviewed changes
Copy link
Contributor

@MaximAlien MaximAlien left a comment

Choose a reason for hiding this comment

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

CarPlay related logic seems to look good.

Udumft
Udumft approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@Udumft Udumft left a comment

Choose a reason for hiding this comment

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

LGTM except current merge conflict and NIT code doc typo above

@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch from d594c57 to 5ef2ef6 Compare September 8, 2021 08:22
@S2Ler S2Ler force-pushed the s2ler/billing-sessions branch from 5ef2ef6 to c05c0d2 Compare September 8, 2021 10:17
@S2Ler S2Ler merged commit bd92453 into main Sep 8, 2021
@S2Ler S2Ler deleted the s2ler/billing-sessions branch September 8, 2021 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@bamx23 bamx23 bamx23 left review comments

@1ec5 1ec5 1ec5 left review comments

@MaximAlien MaximAlien MaximAlien left review comments

@Udumft Udumft Udumft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@S2Ler S2Ler

Labels

None yet

Projects

None yet

Milestone

v2.0.0 (RC)

Development

Successfully merging this pull request may close these issues.

[Bug]: PassiveLocationManager not stopped

5 participants

@S2Ler @bamx23 @1ec5 @MaximAlien @Udumft