Bounty Payout Milestone 1-Review #5027

androolloyd · 2019-08-16T19:35:13Z

Description

New Model:

Organization
- name
- groups (Many to Many relationship to auth.Group)

Model Updates:

Profile
- orgs (Many to Many relationship to dashboard.Organization)

sync_profile updates:

- updated to account for change to orgs referenced above

New Management Command:

- app/management/sync_orgs_repo.py
- no arguments presently
- designed to be run as a long running cron job

Refers/Fixes

#4969

Testing

Organization - name - groups (Many to Many relationship to auth.Group) Model Updates: Profile - organizations (Many to Many relationship to dashboard.Organization) sync_profile updates: - updated to account for change to organizations referenced above New Management Command: - app/management/sync_orgs_repo.py - no arguments presently - designed to be run as a long running cron job

…-payout-4969

- Reverted the change around profile.organizations due to its usage in user hover cards Profile Model - organizations restored - orgs key added for the sync work

codecov · 2019-08-23T18:11:14Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1cb0949). Click here to learn what that means.
The diff coverage is 14.63%.

@@            Coverage Diff            @@
##             master    #5027   +/-   ##
=========================================
  Coverage          ?   17.96%           
=========================================
  Files             ?      207           
  Lines             ?    17473           
  Branches          ?     2399           
=========================================
  Hits              ?     3139           
  Misses            ?    14323           
  Partials          ?       11

Impacted Files	Coverage Δ
app/app/utils.py	`18.51% <0%> (ø)`
app/app/management/commands/sync_orgs_repos.py	`0% <0%> (ø)`
app/app/settings.py	`78.81% <100%> (ø)`
app/dashboard/models.py	`38.28% <48.38%> (ø)`
app/git/utils.py	`14.76% <9.52%> (ø)`

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 1cb0949...e5fc617. Read the comment docs.

codecov · 2019-08-23T18:11:14Z

Codecov Report

Merging #5027 into master will decrease coverage by 0.16%.
The diff coverage is 10.58%.

@@            Coverage Diff             @@
##           master    #5027      +/-   ##
==========================================
- Coverage   29.72%   29.55%   -0.17%     
==========================================
  Files         236      237       +1     
  Lines       20005    20166     +161     
  Branches     2855     2882      +27     
==========================================
+ Hits         5946     5960      +14     
- Misses      13817    13961     +144     
- Partials      242      245       +3

Impacted Files	Coverage Δ
app/app/utils.py	`21.6% <0%> (-0.31%)`	⬇️
app/app/management/commands/sync_orgs_repos.py	`0% <0%> (ø)`
app/app/settings.py	`79.19% <100%> (ø)`	⬆️
app/git/utils.py	`40% <13.04%> (-1.5%)`	⬇️
app/dashboard/models.py	`50.46% <87.5%> (+0.22%)`	⬆️
app/dashboard/views.py	`12.96% <0%> (ø)`	⬆️
...rketing/management/commands/no_applicants_email.py	`0% <0%> (ø)`	⬆️

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 2a2cadc...a03df45. Read the comment docs.

… and not lose permissions

- stale orgs and groups are now purged from users

…-payout-4969

danlipert

Looks good so far - just a few questions and some style guidelines to make our PRs easy to review 👍

app/app/management/commands/sync_orgs_repos.py

danlipert · 2019-08-27T12:46:55Z

app/dashboard/models.py

@@ -121,41 +121,41 @@ def needs_review(self):
        """Filter results by bounties that need reviewed."""


In this file, can you just include the logic changes and ditch all the style changes? I'm guessing you have some kind of automated tool in your IDE or something like that, and while I appreciate the style fix up, different contributors have different style fixing preferences in their IDEs so we get a lot of PRs where style changes are just going back and forth which makes it hard to review.

Noted, will revert those changes, and just add the logic.

… in profile.organizations against profile.orgs

Profile Model Class - repos added - ManyToMany on Repo Organization Class - repos added - ManyToMany on Repo Repos now synced through the sync command

…-payout-4969

octavioamu

Tested on my local but there is a problem with the token so is not taking private orgs, left a comment.

app/git/utils.py

- additional exception handling and error messages to help debugging any issues with user profiles - private organizations should now correctly be capturable when they are granted access during login

- adding a per_page limit of 100 to the calls as a stop gap to proper pagination handling

octavioamu · 2019-10-02T16:22:01Z

def sync_profile(handle, user=None, hide_profile=True) is getting the profile hidden since is by default

app/app/management/commands/sync_orgs_repos.py

- added a missing comma as per the PR, - created a merge migration from the most recent master. Merge branch 'master' of github.com:gitcoinco/web into feature/bounty-payout-4969

octavioamu · 2019-10-22T16:39:29Z

def sync_profile(handle, user=None, hide_profile=True) is getting the profile hidden since is by default

@androolloyd did you found a way to look around this?

…ommand(only fetches active user/profiles)

…ensure we aren't changing their profile display status

app/app/management/commands/sync_orgs_repos.py

thelostone-mc · 2019-10-22T20:48:01Z

app/app/management/commands/sync_orgs_repos.py

+
+                        if 'message' in org_repos:
+                            print(org_repos['message'])
+                            continue


^ remove the if block ?

no, same as above, anytime we see a message key in the returning object, we want to log it and skip to the next element, the error's are vague but are only caused by a couple of things, permissions errors are the most prevelant

app/app/management/commands/sync_orgs_repos.py

androolloyd added 2 commits August 16, 2019 16:25

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

4e3eabb

…-payout-4969

androolloyd requested review from octavioamu and thelostone-mc August 16, 2019 19:35

androolloyd changed the title ~~Bounty Payout Milestone 1~~ Bounty Payout Milestone 1-DRAFT Aug 16, 2019

androolloyd added 2 commits August 22, 2019 22:58

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

db6cedb

…-payout-4969

sync_profile helper

e5fc617

- Reverted the change around profile.organizations due to its usage in user hover cards Profile Model - organizations restored - orgs key added for the sync work

Fix for edge case where users wont have their orgs cleared correctly,…

ddc28c2

… and not lose permissions

androolloyd marked this pull request as ready for review August 24, 2019 00:20

androolloyd requested a review from danlipert as a code owner August 24, 2019 00:20

androolloyd changed the title ~~Bounty Payout Milestone 1-DRAFT~~ Bounty Payout Milestone 1-Review Aug 24, 2019

androolloyd added 4 commits August 26, 2019 11:32

Reconiliation Updated

8bde246

- stale orgs and groups are now purged from users

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

35b09c8

…-payout-4969

migrations added to solve build issues with 4969

4450713

modified previously commited migration to ensure accuracy

e8e5710

danlipert suggested changes Aug 27, 2019

View reviewed changes

androolloyd added 8 commits September 3, 2019 15:56

reverting formatting changes

7f43a79

Fixed an issue with reconciliation, correct the conditional to check…

d78dbe6

… in profile.organizations against profile.orgs

removed stub

526d8bb

filtering user queries based around the is_active flag

3b53af3

adding GithubRepo

8f47727

Repo Model Class Added to dashboard/models

f141e7f

Profile Model Class - repos added - ManyToMany on Repo Organization Class - repos added - ManyToMany on Repo Repos now synced through the sync command

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

6db6e25

…-payout-4969

Merge branch 'master' into feature/bounty-payout-4969

0cdec36

danlipert force-pushed the feature/bounty-payout-4969 branch from 2f3a51b to 58105e6 Compare September 16, 2019 08:17

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

7f41c2a

…-payout-4969

androolloyd force-pushed the feature/bounty-payout-4969 branch from 5323ae1 to 7f41c2a Compare September 16, 2019 12:49

add merge migration

508a81f

Merge branch 'master' of github.com:gitcoinco/web into feature/bounty…

27e75bd

…-payout-4969

octavioamu suggested changes Oct 1, 2019

View reviewed changes

app/git/utils.py Outdated Show resolved Hide resolved

androolloyd and others added 5 commits October 1, 2019 11:16

sync_orgs_repos management cmd

e8ee0b8

- additional exception handling and error messages to help debugging any issues with user profiles - private organizations should now correctly be capturable when they are granted access during login

sync_orgs_repo management command

1831ab9

- adding a per_page limit of 100 to the calls as a stop gap to proper pagination handling

updated the error message to correct the text

6dd7685

add credentials and fix tests

c8d192d

remove typo

a3e1b1a

octavioamu mentioned this pull request Oct 2, 2019

Settings org 2 #5285

Merged

octavioamu suggested changes Oct 22, 2019

View reviewed changes

app/app/management/commands/sync_orgs_repos.py Outdated Show resolved Hide resolved

Bounty Payout

70c4192

- added a missing comma as per the PR, - created a merge migration from the most recent master. Merge branch 'master' of github.com:gitcoinco/web into feature/bounty-payout-4969

androolloyd added 5 commits October 22, 2019 16:30

hide_profile=False is defined for when syncing a profile inside the c…

4ff4d69

…ommand(only fetches active user/profiles)

mend

7166b6d

sync_profile updated to check for profile existing before syncing to …

b5f05cd

…ensure we aren't changing their profile display status

mend

01c2e6a

mend

3fd2ce1

octavioamu requested review from danlipert and thelostone-mc October 22, 2019 20:14

octavioamu approved these changes Oct 22, 2019

View reviewed changes

thelostone-mc reviewed Oct 22, 2019

View reviewed changes

app/app/management/commands/sync_orgs_repos.py Show resolved Hide resolved

thelostone-mc reviewed Oct 22, 2019

View reviewed changes

app/app/management/commands/sync_orgs_repos.py Show resolved Hide resolved

thelostone-mc reviewed Oct 22, 2019

View reviewed changes

app/app/management/commands/sync_orgs_repos.py Show resolved Hide resolved

danlipert approved these changes Oct 23, 2019

View reviewed changes

thelostone-mc approved these changes Oct 23, 2019

View reviewed changes

Merge branch 'master' into feature/bounty-payout-4969

a03df45

octavioamu merged commit b9c515f into master Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bounty Payout Milestone 1-Review #5027

Bounty Payout Milestone 1-Review #5027

androolloyd commented Aug 16, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

danlipert left a comment

danlipert Aug 27, 2019

androolloyd Sep 3, 2019

octavioamu left a comment

octavioamu commented Oct 2, 2019

octavioamu commented Oct 22, 2019

thelostone-mc Oct 22, 2019

androolloyd Oct 23, 2019

		@@ -121,41 +121,41 @@ def needs_review(self):
		"""Filter results by bounties that need reviewed."""

Bounty Payout Milestone 1-Review #5027

Bounty Payout Milestone 1-Review #5027

Conversation

androolloyd commented Aug 16, 2019 • edited Loading

Description

Refers/Fixes

Testing

codecov bot commented Aug 23, 2019

Codecov Report

codecov bot commented Aug 23, 2019 • edited Loading

Codecov Report

danlipert left a comment

Choose a reason for hiding this comment

danlipert Aug 27, 2019

Choose a reason for hiding this comment

androolloyd Sep 3, 2019

Choose a reason for hiding this comment

octavioamu left a comment

Choose a reason for hiding this comment

octavioamu commented Oct 2, 2019

octavioamu commented Oct 22, 2019

thelostone-mc Oct 22, 2019

Choose a reason for hiding this comment

androolloyd Oct 23, 2019

Choose a reason for hiding this comment

androolloyd commented Aug 16, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019 •

edited

Loading