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

Stats: apply API changes to prevent reset #2541

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

leandroalonso
Copy link
Member

@leandroalonso leandroalonso commented Dec 12, 2024

We had a few instances of users' complaining that their Stats reset or reduced. This PR applies the latest changes deployed on Staging ensuring:

  1. That the backend never "reduces" (or reset) any Stat;
  2. That we pull back the stats in case we need

To test

  1. First, change the Build Configuration to Staging and run the app
  2. Login to a staging account
  3. Ensure this account has some Stats. If it doesn't, manipulate these lines and trigger a refresh (just add numbers there)
  4. Then, remove all the updateQueue.sync block from the StatsManager init method. This will simulate having negative stats
  5. Go to Stats
  6. ✅ Reduced Stats should appear but it should update to the correct one
  7. Re-run the app
  8. Trigger a refresh from Profile
  9. Go to Stats
  10. ✅ Stats should be correct

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.
@@ -29,6 +29,39 @@ public class StatsManager {
}
}

func updateStatsIfNeeded(savedDynamicSpeed: TimeInterval, savedVariableSpeed: TimeInterval, totalListenedTo: TimeInterval, totalSkipped: TimeInterval, savedAutoSkipping: TimeInterval) {
let minimumStatsChangeToUpdate = 100
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm debating whether this should be 100 or something bigger or smaller.

Maybe if the local stat is just smaller than remote we should update?

@leandroalonso leandroalonso marked this pull request as ready for review December 12, 2024 18:12
@leandroalonso leandroalonso requested a review from a team as a code owner December 12, 2024 18:12
@leandroalonso leandroalonso requested review from SergioEstevao and removed request for a team December 12, 2024 18:12
let minimumStatsChangeToUpdate: TimeInterval = 100

updateQueue.sync {
if savedDynamicSpeed - self.savedDynamicSpeed > minimumStatsChangeToUpdate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do you want to save only if the difference is positive right? No need to use abs ?

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Followed the test instructions and the stats recovered to the correct values.
Just left a small nit that you may want to consider, but feel free to ignore if does not make sense for the scenario.
:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants