-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
New bounty creation flow #10409
New bounty creation flow #10409
Conversation
6fca651
to
9e637b3
Compare
967afc1
to
2ab5908
Compare
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
fd3d6b3
to
bbc94be
Compare
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
910a614
to
dd5bec5
Compare
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
The new docker image for has been pushed to: |
f07cead
to
74853f7
Compare
035a188
to
849c2df
Compare
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.
left a couple of comments.
Will review the rest later today as this quite a big PR :P
app/app/urls.py
Outdated
@@ -443,6 +443,7 @@ | |||
), | |||
|
|||
# View Bounty | |||
# TODO geri: should we drop the 2 URL patterns below??? |
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.
Is this something which needs to be addressed now ?
would it make more sense to track this in a ticket as opposed to the code ?
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 was reluctant to remove this to avoid breaking the links that users have ....
I will write a proper comment here.
@@ -454,6 +455,7 @@ | |||
name='issue_details_new2' | |||
), | |||
re_path(r'^funding/details/?', dashboard.views.bounty_details, name='funding_details'), | |||
re_path(r'^issue/(?P<bounty_id>\d+)', dashboard.views.bounty_details, name='issue_details_new4'), |
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.
couldn't we remove the old ones issue_details_new2 / is that still needed ?
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 would not remove this now, see my comment above.
let vm = this; | ||
let ret = false; |
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.
rename this to something more meaningful ?
for (let i = 0; i < vm.bounty.owners.length; i++) { | ||
let additionalOwner = vm.bounty.owners[i]; | ||
|
||
console.log('geri: additionalOwner', additionalOwner.handle); |
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 the log ?
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.
No, I will remove this
app/retail/views.py
Outdated
@@ -19,6 +19,7 @@ | |||
''' | |||
import json | |||
import logging | |||
from pprint import pformat |
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.
Is this needed?
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.
No, I will remove this
@@ -7,6 +7,7 @@ <h4>[[heading]]</h4> | |||
</template> | |||
<slot></slot> | |||
</div> | |||
<slot name="footer"></slot> |
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.
what's this for ?
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.
This is for additional content that we display below the wizard, if it is the case.
We have a use case where we display fee information for bounty payments in the payment step only.
usd_pegged_value_in_token_now = models.DecimalField(default=0, decimal_places=2, max_digits=50, blank=True, null=True) # The calculated amount in token, corresponding to value_true_usd | ||
usd_pegged_value_in_token = models.DecimalField(default=0, decimal_places=2, max_digits=50, blank=True, null=True) # The calculated amount in token, corresponding to value_true_usd | ||
value_true_usd = models.DecimalField(default=0, decimal_places=2, max_digits=50, blank=True, null=True) # The value the user wants to pay in USD | ||
peg_to_usd = models.BooleanField(default=False) # True if the amount to pay should be pegged to USD |
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.
^ can none of the above fields like value_in_usdt etc be resued ? Just curious
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.
This is probably possible, but I think having this additional variables makes following the logic flow much easier:
- if the user does not peg to USD -> we use the old vars, and conversions are made from token amount -> usd amount
- if the user does peg to USD -> we use the new vars, and conversions are made from usd amount -> token amount
I think this helps understand better which value the user entered: value_true respectively value_true_usd.
app/dashboard/admin.py
Outdated
@@ -379,7 +379,7 @@ class BountyAdmin(admin.ModelAdmin): | |||
raw_id_fields = ['interested', 'coupon_code', 'org', 'event', 'bounty_owner_profile', 'bounty_reserved_for_user'] | |||
ordering = ['-id'] | |||
|
|||
search_fields = ['raw_data', 'title', 'bounty_owner_github_username', 'token_name'] | |||
search_fields = ['raw_data', 'title', 'bounty_owner_github_username', 'token_name', 'cusom_title', 'custom_description'] |
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.
cusom_title -> custom_title
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 will fix this.
- redirect to new url format from bounty invitation - fix failure loading details page due to invalid JSON in acceptance criteria and ressources - ®emved hardcoded URLs for network icons
fd9c29f
to
b537b20
Compare
b537b20
to
7e25aee
Compare
Description
This implements the new bounty creation flow.
Refers/Fixes
Se this board for the issue that this PR fixes:
https://github.com/orgs/gitcoinco/projects/4/views/1?filterQuery=epic%2Finitiative%3A%22Bounty+Creation%22
Testing