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.

Sign up for GitHub

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

Jump to bottom

Add remove tribe on townsquare #7152

Merged
merged 9 commits into from
Aug 3, 2020
Merged

Add remove tribe on townsquare #7152

merged 9 commits into from
Aug 3, 2020

Conversation

zoek1
Copy link
Contributor

@zoek1 zoek1 commented Jul 27, 2020

Description

https://www.loom.com/share/8f3829efefc14c87a731c14bd8a2fd40

  • removes that tribe from the DOM
  • sends a PUT reuqest to the backend which stores an 'IgnoredSuggestedTribe' object in the DB
  • refreshes the suggested tribes module with 5 new tribes
  • next pageload, the X'd out tribe will not be loaded again
Refers/Fixes

#6424

Testing

@codecov
Copy link

codecov bot commented Jul 27, 2020 •
edited
Loading

Codecov Report

Merging #7152 into master will increase coverage by 0.03%.
The diff coverage is 38.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7152      +/-   ##
==========================================
+ Coverage   26.21%   26.24%   +0.03%     
==========================================
  Files         296      296              
  Lines       29258    29277      +19     
  Branches     4321     4323       +2     
==========================================
+ Hits         7669     7683      +14     
- Misses      21313    21323      +10     
+ Partials      276      271       -5
Impacted Files Coverage Δ
app/app/urls.py 85.96% <ø> (ø)
app/townsquare/views.py 10.55% <35.00%> (+1.05%) ⬆️
app/dashboard/models.py 48.54% <100.00%> (+0.01%) ⬆️
app/quests/views.py 16.22% <0.00%> (ø)
app/dashboard/views.py 10.48% <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/embed.py 31.60% <0.00%> (+3.44%) ⬆️

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 7afbf87...ffbd6cf. Read the comment docs.

danlipert
danlipert suggested changes Jul 28, 2020
View reviewed changes
app/dashboard/models.py Outdated
@@ -2788,7 +2788,7 @@ class Profile(SuperModel):
last_validation_request = models.DateTimeField(blank=True, null=True, help_text=_("When the user requested a code for last time "))
encoded_number = models.CharField(max_length=255, blank=True, help_text=_('Number with the user validate the account'))
sybil_score = models.IntegerField(default=-1)

ignore_tribes = ArrayField(models.IntegerField(), blank=True, default=list)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use an ArrayField here instead of a ManyToManyField? Will cause problems if a tribe is later removed and we aren't cascading the deletion down to the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the ArrayField because the spec in the issue requests for one object to store the ignored tribes. I got your point, at this moment doesn't matter if a tribe is deleted because all ids in this list will be excluded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the ArrayField because the spec in the issue requests for one object to store the ignored tribes. I got your point, at this moment doesn't matter if a tribe is deleted because all ids in this list will be excluded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with ManyToMany

@danlipert danlipert marked this pull request as draft July 28, 2020 10:06
@zoek1 zoek1 requested a review from danlipert July 29, 2020 14:28
@zoek1 zoek1 marked this pull request as ready for review July 29, 2020 14:28
@danlipert
Copy link
Contributor

@zoek1 Conceptually looks good - looks like Travis doesn't like it, possibly the model setup or migrations, please take a look

@zoek1
Copy link
Contributor Author

zoek1 commented Jul 30, 2020

Conflicts fixed @danlipert

@danlipert danlipert merged commit 483f70c into gitcoinco:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@octavioamu octavioamu octavioamu approved these changes

@danlipert danlipert danlipert approved these changes

@frankchen07 frankchen07 Awaiting requested review from frankchen07

@PixelantDesign PixelantDesign Awaiting requested review from PixelantDesign

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zoek1 @danlipert @octavioamu @thelostone-mc