Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Adding decimal place for time saved stats #7489

Merged
merged 1 commit into from
Mar 11, 2017

Conversation

maazadeeb
Copy link
Contributor

@maazadeeb maazadeeb commented Mar 4, 2017

Fixes #6650

Time saved in hours will now have 1 decimal place. And time saved
in days will have 2 decimal places

Auditors: @bbondy @bsclifton

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan

  1. Make sure all instances of Brave are closed
  2. Open your session file (~/Libraries/Application Support/brave/session-state-1 on macOS, %appdata%\brave\session-state-1 on Windows) in an editor such as Sublime, vim, notepad, etc. If you have a JSON formatting plugin, you may wish to use it (so that it's easier to edit)
  3. Edit the JSON foradblock > count to be 555555 (six fives)
    screen shot 2017-03-10 at 4 58 46 pm
  4. Save the file and then launch Brave
  5. Verify it shows hours with 1 decimal point of precision, like so:
    screen shot 2017-03-10 at 4 58 12 pm
  6. Close Brave and re-open your session file in your editor of choice
  7. Edit the adblock > count value to be 55555555 (eight fives)
  8. Save the file and then launch Brave
  9. Verify it shows the days with two digits of precision, like so:
    image

Fixes brave#6650

Time saved in hours will now have 1 decimal place. And time saved
in days will have 2 decimal places

Auditors: @bbondy @bsclifton
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Great work, @maaz93! 😄 I just tested this and it looks perfect. I added testing steps to make it easier for the manual testers.

Huge 👍 for adding the unit tests- very much appreciated 😄

@bsclifton bsclifton merged commit e151a4e into brave:master Mar 11, 2017
@bsclifton
Copy link
Member

bsclifton commented Mar 11, 2017

@maaz93 if you're interested, another very similar issue (but more difficult) would be #5889

When testing the above and putting a HUGE number of ads in place (via my session), I noticed it overflow like so:
screen shot 2017-03-10 at 4 57 00 pm

The actual number is 55,555,555 which may be abbreviated as 55.5 million. This issue would be more difficult because you'd need to pull the units out (or their abbreviation, like 10k for 10,000) into the locale files, so that other locales can add their appropriate name/symbol

@maazadeeb
Copy link
Contributor Author

Thanks @bsclifton ! 😄 The issue that you've mentioned is interesting. I would like to take it up. I might need some info regarding it. I'll post it on the issue itself, after going through it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixed "Estimated Time Saved" to take minutes into account
3 participants