-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
WIP: Update FaucetRequest with FK to Profile #884
Conversation
app/faucet/views.py
Outdated
@@ -90,12 +92,12 @@ def save_faucet(request): | |||
}, status=400) | |||
fr = FaucetRequest.objects.create( | |||
fulfilled=False, | |||
github_username=github_profile, | |||
input_github_username=profile_handle, | |||
github_username=request.user.username, | |||
github_meta=checkeduser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owocki Do we need or want the GH payload here? We could eliminate the additional call to GH entirely if we don't need github_meta
/checkeduser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think we need it no
Codecov Report
@@ Coverage Diff @@
## master #884 +/- ##
==========================================
+ Coverage 31.48% 31.56% +0.08%
==========================================
Files 106 106
Lines 7214 7227 +13
Branches 939 939
==========================================
+ Hits 2271 2281 +10
- Misses 4833 4836 +3
Partials 110 110
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didnt test but LGTM -- pls just test with both logged in / out before merging
app/faucet/views.py
Outdated
@@ -90,12 +92,12 @@ def save_faucet(request): | |||
}, status=400) | |||
fr = FaucetRequest.objects.create( | |||
fulfilled=False, | |||
github_username=github_profile, | |||
input_github_username=profile_handle, | |||
github_username=request.user.username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be wrapped around request.user.is_authenticated()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this case. We're validating that profile
is present on L67 which relies on L65 that checks if request.user.is_authenticated
, otherwise the user will receive a 401.
migrations.AddField( | ||
model_name='faucetrequest', | ||
name='profile', | ||
field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='faucet_requests', to='dashboard.Profile'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E501 line too long (149 > 120 characters)
…6-296 * 'master' of https://github.com/gitcoinco/web: (31 commits) fixes https://rollbar.com/gitcoin/gitcoin/items/908 dashboard.js -- onboard search was expecing falsy value, not -1 measure hourly rates of different repos Fix url collision Restrict onboarding view to staff temporarily sets default search project status to 'any' (gitcoinco#936) staggers cron start times (gitcoinco#1034) dont show email address if not auth fixes gitcoinco#1025 obfuscate usernames review: added feedback review: added feedback onboard: refactored code dashboard: handle redirect from onboard ftux: birth WIP: Update FaucetRequest with FK to Profile (gitcoinco#884) fixes tests no more underscore methods for non-private methods Fix gitcoinco#1032 - Resolve trans rendering for faucet request copy allow user to skip waiting for tip tx to clear blockchain ...
Description
The goal of this PR is to deprecate some of the old user handling in the Faucet related views and add a FK relation between
FaucetRequest
andProfile
.Checklist
Affected core subsystem(s)
faucet, profile