-
-
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
Remove misleading buttons on gitcoin UI #112
Conversation
It seems that the build failed because it could not load the plugin 'ethereum'. I'm not familiar with the Ether ecosystem to know why that might happen in a Travis CI test (and if my changes may be responsible for that), and I don't know how to kick off another build of Travis CI to see if this is just a temporary issue. |
there is some chatter in the dev-python room about this |
} | ||
|
||
var isBountyOwner = function(result) { | ||
var bountyAddress = result['bounty_owner_address'] | ||
return (web3 && (web3.eth.coinbase == bountyAddress)) |
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.
make sure you make this comparison case insensitive!
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 are other places in the codebase where we aren't doing case-insensitive comparison checks (for example, here). If you want, I can probably find all these instances and replace them with case-insensitive comparison cases...I just want to confirm first to make sure that case-insensitive comparison cases is indeed what we want.
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.
ah shoot.. i didnt realize this.
i dont think making these updates as part of this PR makes sense. thanks for the offer tho.
var entry = { | ||
href: '/unwatch', | ||
text: 'Unwatch', | ||
parent: 'left_actions', | ||
color: 'darkGrey' | ||
} | ||
actions.push(entry); | ||
} else { | ||
} else if (canWatch) { |
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.
seems to me that anyone shoudl be able to watch an issue. even bountie owners.. no?
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 problem with that is that it doesn't really matter if a bounty owner is watching an issue or not. When I view the issue in the /explore
dashboard, the bounty owner's issue is automatically marked with the word "Mine", so it is impossible to ever see "Watched".
See this JavaScript snippet where we decide whether to generate what button to place next to a bounty on the dashboard ("Mine", "Watched", etc.). It seems this is the only place where we actually use the Watch list.
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 bounty owner's issue is automatically marked with the word "Mine", so it is impossible to ever see "Watched".
got it; this is a UI issue that we should fix.
note that in the sidebar filters it is possible to filter only by your 'watched' issues... so there is some functional difference here though.
we should def fix the UI thing you noticed above eventually
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.
We are currently generating some buttons on the gitcoin UI by first querying information about the bounty and then deciding what buttons to add on the UI based on that info. However, we are only generating these buttons based on the status of the bounty on the blockchain, and we do not take into account whether the user is supposed to see those buttons. (For example, it makes little sense for a person who just created a bounty to then claim their own bounty.) To prevent users from seeing buttons that they are not meant to see, we will use the 'bounty_owner_address' to decide whether somebody is a "Bounty Owner". If a user is currently logged into Metamask and have the same address as the 'bounty_owner_address', then they are likely the Bounty Owner. We then use this knowledge (in addition to the current status of the bounty) to decide what buttons we should generate. Bounty owners cannot claim their own issues. Non-"bounty owners" cannot accept/reject issues or clawback funds.
0f0e5f4
to
8fcb99c
Compare
I undid the change for watching, and will leave the case-insensitive issue as another issue. Now kicking off the build. |
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
======================================
Coverage 12.3% 12.3%
======================================
Files 65 65
Lines 3073 3073
Branches 340 340
======================================
Hits 378 378
Misses 2695 2695 Continue to review full report at Codecov.
|
🎉 thanks @tra38 ! |
Remove misleading buttons on gitcoin UI
workarround wallet.js / richards codekit scss build pipeline
PR is being submitted for #81.
We are currently generating some buttons on the gitcoin UI
by first querying information about the bounty and then deciding
what buttons to add on the UI based on that info.
However, we are only generating these buttons based on the
status of the bounty on the blockchain, and we do not take into account
whether the user is supposed to see those buttons. (For example, it makes
little sense for a person who just created a bounty to then claim their
own bounty.)
To prevent users from seeing buttons that they are not meant to see,
we will use the 'bounty_owner_address' to decide whether somebody
is a "Bounty Owner". If a user is currently logged into Metamask
and have the same address as the 'bounty_owner_address',
then they are likely the Bounty Owner. We then use this knowledge (in addition
to the current status of the bounty) to decide what buttons we should generate.
Bounty owners cannot watch or claim their own issues.
Non-"bounty owners" cannot accept/reject issues or clawback funds.
WARNING - Though this change will remove the "Claim" button, it does not stop "bounty owners" from actually claiming their own issue, simply by visiting '/funding/claim' and then typing in the URL of the GitHub issue.
I know we have some code in
clawback_expired_bounty.js
andprocess_bounty.js
that checks if an action is valid, and I am tempted to use that same code inclaim_bounty.js
, but I want to get your confirmation first. There might be some rare instances where a "bounty owner" might want to claim their own bounty (example: to indicate that the bounty is unavailable).