Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding option to remove sites from auto-contribute #420

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Sep 7, 2018

Addresses: brave/brave-browser#967

Native ledger work:
brave-intl/bat-native-ledger#87
brave-intl/bat-native-ledger#93

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).
  • 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/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

  1. Launch Brave, enable rewards
  2. Visit 6 sites to populate contributions table
  3. In the contribution table, click See all ${n} sites to display the removal modal
  4. Click the x icon next to a few publishers
  5. Confirm they are excluded and removed from the contribution table

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

@ryanml ryanml self-assigned this Sep 7, 2018
@bbondy bbondy changed the title Adding option to remove sites from auto-contribute WIP: Adding option to remove sites from auto-contribute Sep 8, 2018
@ryanml ryanml force-pushed the remove-sites branch 4 times, most recently from fea159b to e54ea8b Compare September 11, 2018 01:29
@ryanml ryanml changed the title WIP: Adding option to remove sites from auto-contribute Adding option to remove sites from auto-contribute Sep 11, 2018
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

When I remove a publisher, the modal list updates but the autocontribute box never updates even across restarts:
screen shot 2018-09-11 at 8 04 34 am
I've removed sites to leave only 2 but autocontribute says I still have 8.

@ryanml
Copy link
Contributor Author

ryanml commented Sep 11, 2018

@jasonrsadler good catch, will fix

@ryanml ryanml closed this Sep 11, 2018
@ryanml ryanml reopened this Sep 11, 2018
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

++ Good job! Works good!

@jasonrsadler jasonrsadler merged commit 7564b7c into brave:master Sep 11, 2018
jasonrsadler pushed a commit that referenced this pull request Sep 11, 2018
Adding option to remove sites from auto-contribute
@jasonrsadler
Copy link
Contributor

master: 7564b7c
0.55.x: d8716b0

@bbondy
Copy link
Member

bbondy commented Sep 11, 2018

For next time pls also add 0.55.x here as an indication that it's the first channel it appears on. I review a list of PRs periodically to make sure we didn't forget to uplift some things. I added it this time 👍

@bbondy bbondy added the 0.55.x label Sep 11, 2018
@@ -39,11 +39,15 @@ class ContributeBox extends React.Component<Props, State> {
}
}

getContributeRows = (list: Rewards.Publisher[]) => {
getIncludedPublishers = (list: Rewards.Publisher[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done on DB level

@ryanml
Copy link
Contributor Author

ryanml commented Sep 16, 2018

@NejcZdovc I need to be able to show the number of excluded sites for contribute modals restore option, if we fetch only the included publishers, I won't be able to calculate the difference. What do you suggest? Fetching only the count?

@ryanml
Copy link
Contributor Author

ryanml commented Sep 16, 2018

@NejcZdovc - you want to see something like:

export type Excluded = 'DEFAULT' | 'EXCLUDED' | 'INCLUDED' ?

@NejcZdovc
Copy link
Contributor

@ryanml something like this

export enum Excluded {
    DEFAULT = 0,
    INCLUDED = 1,
    EXCLUDED = 2
}

@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants