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

Refactors database #921

Merged
merged 14 commits into from
Jan 16, 2019
Merged

Refactors database #921

merged 14 commits into from
Jan 16, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Nov 16, 2018

Resolves brave/brave-browser#2163
Resolves brave/brave-browser#2551

Native ledger: brave-intl/bat-native-ledger#183

Submitter Checklist:

  • 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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  • general regression of rewards
  • focused on adding things to table and contribution
  • make sure that upgrade path (form previous version) is working correctly, that no data is lost
  • delete some sites and then restore them, make sure that excluded num is tracked correctly
  • try doing ac and make sure that all items from the table are send out

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

bbondy
bbondy previously requested changes Nov 20, 2018
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Please add a test to cover what's being refactored.

@NejcZdovc
Copy link
Contributor Author

this is done, @tmancey is just adding tests and then we are good to go

@NejcZdovc
Copy link
Contributor Author

note: tests are still failing, but are not failing because of the code but setup, so PR code is ready for a review while I am working with @tmancey on test setup

@NejcZdovc NejcZdovc force-pushed the publisher-refactor branch 3 times, most recently from 7138068 to d4ef111 Compare December 20, 2018 11:10
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

I did a general Rewards regression using this feature, and overall things are smooth. No errors or warnings that I can see.

*Items are added to the AC table as expected, respecting settings
*Items can be removed and restored as expected, excluded site counts are correct
*Contribution processed as expected when a command line flag to shorten the time was provided.

I did notice this, and am unsure if it's related to a prior issue:
I have two sites, a.com and b.com in the ac table. The first has 71% attention, the second 29%. I go to remove b.com and we are left with a.com at 100%, which is correct. However, when I restore the table, both sites are listed as 100% attention.

@NejcZdovc
Copy link
Contributor Author

@ryanml fixes 100% problem and finish with database tests, ready for re-review

@bridiver
Copy link
Collaborator

@NejcZdovc can you provide an overview of the changes? It's quite a large PR so a summary of the changes would be helpful

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jan 15, 2019

@bridiver Change is tied to splitting publisher api. Before there was only one api for publisher and activity, where now we have dedicated publisher api (which updates publisher_info) and activity api (which updates publisher_info and activity_info). This way you don't need to provide filters and all other stuff when you just want to update for example exclusion flag for the publisher.

It also optimized save for visited site, so if you have duration 0 we only insert into publisher_info (using new api) and not populating activity_info with data that is 0.

This PR also addresses restore problem that we had, where we were sending restore publisher from exclusion list one by one to the db, where now we restore all of them in one sql call.

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

++ confirmed attention re-calculation issue is fixed, good work!

@NejcZdovc NejcZdovc added this to the 0.61.x - Nightly milestone Jan 16, 2019
@NejcZdovc NejcZdovc merged commit 0ee2f71 into master Jan 16, 2019
@NejcZdovc NejcZdovc deleted the publisher-refactor branch January 16, 2019 06:00
NejcZdovc added a commit that referenced this pull request Jan 16, 2019
@NejcZdovc NejcZdovc mentioned this pull request Jan 16, 2019
@jasonrsadler
Copy link
Contributor

e3c864e#diff-4bf8d740c793a0cf750bc637b07f65a0R418

Should these have been included?

@NejcZdovc
Copy link
Contributor Author

@jasonrsadler nope, this log shouldn't be there, will remove it

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

Successfully merging this pull request may close these issues.

Restore all from deleted publisher doesn't restore all - Follow up to 1441 Refactor database
6 participants