-
-
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
fix for empty github notification #622
Conversation
@@ -297,6 +297,9 @@ def build_profile_pairs(bounty): | |||
""" | |||
profile_handles = [] | |||
for fulfillment in bounty.fulfillments.select_related('profile').all().order_by('pk'): | |||
if fulfillment.profile and fulfillment.profile.handle and fulfillment.profile.absolute_url: | |||
if fulfillment.profile and fulfillment.profile.handle.strip() and fulfillment.profile.absolute_url: |
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.
should make " " falsy
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.
Do we need .strip()
here?
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.
the reason i did that is that this comment ( http://bits.owocki.com/0m450K2E0S3t/Screen%20Shot%202018-03-13%20at%206.01.19%20PM.png ) seems to have whitespace in it.. otherwise how did it pass the fulfillment.profile and fulfillment.profile.handle and fulfillment.profile.absolute_url
check?
Codecov Report
@@ Coverage Diff @@
## master #622 +/- ##
==========================================
- Coverage 33.99% 33.97% -0.02%
==========================================
Files 92 92
Lines 5051 5053 +2
Branches 584 584
==========================================
Hits 1717 1717
- Misses 3267 3269 +2
Partials 67 67
Continue to review full report at Codecov.
|
@@ -297,6 +297,9 @@ def build_profile_pairs(bounty): | |||
""" | |||
profile_handles = [] | |||
for fulfillment in bounty.fulfillments.select_related('profile').all().order_by('pk'): | |||
if fulfillment.profile and fulfillment.profile.handle and fulfillment.profile.absolute_url: | |||
if fulfillment.profile and fulfillment.profile.handle.strip() and fulfillment.profile.absolute_url: |
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.
Do we need .strip()
here?
profile_handles.append((fulfillment.profile.handle, fulfillment.profile.absolute_url)) | ||
else: | ||
addr = f"https://etherscan.io/address/{fulfillment.fulfiller_address}" | ||
profile_handles.append((fulfillment.fulfiller_address, addr)) |
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.
👍
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.
lgtm
Description
fix for
http://bits.owocki.com/0m450K2E0S3t/Screen%20Shot%202018-03-13%20at%206.01.19%20PM.png
JoinColony/colonyNetwork#174 (comment)
Checklist
Affected core subsystem(s)
notifications
Testing
tested it