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

Add Twitter Verified Combinations & Trust Bonus #7537

Merged
merged 5 commits into from
Nov 10, 2020
Merged

Conversation

frankchen07
Copy link
Contributor

@frankchen07 frankchen07 commented Sep 25, 2020

Description

Added Twitter Verified handling (f6e3365).
Simplified combination loops for different verification statuses (3c3e18f).
Accounting for Trust Bonus data model, which overwrites and nullifies both previous commits (4d20cb4).

Refers/Fixes
  1. No reference, added twitter verification, and we need to be able to handle it.
  2. Adding the Trust Bonus data model to massively simplify handling of combinations.
Testing

any and for logic tested with 3 test cases:

x = [1, 2, 3]

In [10]: 9 > 1 and any(i in x for i in [9, 1])
Out[10]: True

In [11]: 9 > 6 and any(i in x for i in [9, 6])
Out[11]: False

In [12]: 3 > 1 and any(i in x for i in [3, 1])
Out[12]: True

In [13]: 4 > 1 and any(i in x for i in [4, 1])
Out[13]: True

In [14]: 9 > 7 and any(i in x for i in [9, 7])
Out[14]: False

With 4d20cb4, the trust bonus data model massively simplifies the logic above (and eliminates the need for it).

closes #7821

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #7537 into stable will increase coverage by 0.92%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           stable    #7537      +/-   ##
==========================================
+ Coverage   26.00%   26.92%   +0.92%     
==========================================
  Files         310      310              
  Lines       31206    33968    +2762     
  Branches     4618     5675    +1057     
==========================================
+ Hits         8114     9146    +1032     
- Misses      22813    24429    +1616     
- Partials      279      393     +114     
Impacted Files Coverage Δ
app/grants/clr.py 0.00% <0.00%> (ø)
app/dashboard/search_indexes.py 97.53% <0.00%> (-1.14%) ⬇️
app/grants/tasks.py 17.29% <0.00%> (-0.82%) ⬇️
app/quests/views.py 16.22% <0.00%> (ø)
...arketing/management/commands/new_bounties_email.py 0.00% <0.00%> (ø)
...rketing/management/commands/no_applicants_email.py 0.00% <0.00%> (ø)
...eting/management/commands/assemble_leaderboards.py 39.73% <0.00%> (ø)
app/dashboard/tasks.py 24.87% <0.00%> (+0.05%) ⬆️
app/dashboard/views.py 11.66% <0.00%> (+1.17%) ⬆️
app/marketing/mails.py 14.92% <0.00%> (+2.78%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8655f44...4d20cb4. Read the comment docs.

@frankchen07 frankchen07 changed the title Add Twitter Verified Combinations Add Twitter Verified Combinations & Trust Bonus Sep 28, 2020
@thelostone-mc
Copy link
Member

@frankchen07 what's the status on this ? Good to review ?

@frankchen07
Copy link
Contributor Author

@thelostone-mc yes it should be ready. it was to account for kevin's trust bonus field, which would eliminate the need to pull verified lists, we just have to persist the trust bonus score through the CLR calculation (which is done in the latest commit) in this PR.

@thelostone-mc thelostone-mc changed the base branch from stable to master November 5, 2020 11:29
app/grants/clr.py Outdated Show resolved Hide resolved
@frankchen07
Copy link
Contributor Author

@thelostone-mc, ready for review. Big +1 for @octavioamu for sitting with me and helping me figure things out on git and vscode.

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

codewise lgtm

@thelostone-mc
Copy link
Member

@frankchen07 was this tested with data or do we need to so that ?

@thelostone-mc
Copy link
Member

synced up with @frankchen07 ! it has been tested

@thelostone-mc
Copy link
Member

locally verified as well

@thelostone-mc thelostone-mc merged commit 8322631 into master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add trust bonus code PR
2 participants