Skip to content

Conversation

@Guardiola31337
Copy link
Contributor

Description

Fixes notification and banner ETAs not in sync
Fixes #1887

Goal

This was a regression from #1412 where we updated the ETAs to consume NN ones but we forgot to change in MapboxNavigationNotification. This PR solves this

Implementation

In MapboxNavigationNotification fix the duration remaining from routeProgress.durationRemaining() to routeProgress.currentLegProgress().durationRemaining() so both the SummaryModel and MapboxNavigationNotification use the same value. Took the opportunity to refactor MapboxNavigationNotification and make it more testable

Screenshots or Gifs

Before

wrong_etas

After

wrong_etas_fixed

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested locally (including SNAPSHOT upstream dependencies if needed)
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code

@Guardiola31337 Guardiola31337 added this to the v0.36.0 milestone Apr 15, 2019
@Guardiola31337 Guardiola31337 self-assigned this Apr 15, 2019
@codecov-io
Copy link

Codecov Report

Merging #1889 into master will increase coverage by 0.45%.
The diff coverage is 61.11%.

@@             Coverage Diff              @@
##             master    #1889      +/-   ##
============================================
+ Coverage     33.86%   34.31%   +0.45%     
- Complexity     1031     1042      +11     
============================================
  Files           257      257              
  Lines          8781     8791      +10     
  Branches        665      666       +1     
============================================
+ Hits           2974     3017      +43     
+ Misses         5545     5509      -36     
- Partials        262      265       +3

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@Guardiola31337 this looks good, thanks for adding the tests here as well 🚢

@Guardiola31337 Guardiola31337 merged commit 5a3b9b1 into master Apr 16, 2019
@Guardiola31337 Guardiola31337 deleted the pg-1887-wrong-etas branch April 16, 2019 13:09
@Guardiola31337 Guardiola31337 mentioned this pull request Apr 17, 2019
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notification and banner ETAs not in sync

3 participants