Skip to content
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

Grant categories for new grants #5615

Merged
merged 11 commits into from
Feb 20, 2020

Conversation

vince0656
Copy link
Contributor

@vince0656 vince0656 commented Dec 8, 2019

Description

Work to enable grant categories to be defined on creation of a grant. A user can specify multiple categories for a grant which then appear on the grant's card on the grant explorer and can be edited later on the details page. Users can now also select categories on the grant explorer and view all grants that relate to that category.

Refers/Fixes

#5456

Testing

Appropriate unit tests have been added

Video Demonstration

I have separated this in two parts. Firstly, creation and updating of both types of grants as follows:
https://drive.google.com/open?id=1YeoU0InrJAnDGLYciKBoC8egZ1NA_o7a

and then category filtering on the grant explorer as follows:
https://drive.google.com/open?id=1FHwHSiSgt4AxF4y-YUrYmH_1pKEQCHkt

It's taken a while to get to the point where all of these features are robust and stable so I'm very happy with where the code is at this point

@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #5615 into master will decrease coverage by 0.51%.
The diff coverage is 25.91%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5615      +/-   ##
=========================================
- Coverage   29.51%     29%   -0.52%     
=========================================
  Files         265     271       +6     
  Lines       22912   23713     +801     
  Branches     3331    3450     +119     
=========================================
+ Hits         6763    6877     +114     
- Misses      15871   16562     +691     
+ Partials      278     274       -4
Impacted Files Coverage Δ
app/app/settings.py 80.06% <ø> (ø) ⬆️
app/bounty_requests/admin.py 100% <ø> (ø) ⬆️
app/quests/admin.py 46.55% <0%> (-0.82%) ⬇️
app/kudos/models.py 51.71% <0%> (-0.72%) ⬇️
app/kudos/admin.py 60.24% <0%> (-0.74%) ⬇️
...eting/management/commands/assemble_leaderboards.py 42.2% <0%> (-21.04%) ⬇️
app/quests/models.py 41.89% <0%> (-0.96%) ⬇️
...board/management/commands/create_search_results.py 0% <0%> (ø)
app/retail/emails.py 22.45% <0%> (-0.82%) ⬇️
app/grants/utils.py 19.04% <0%> (-0.96%) ⬇️
... and 46 more

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 aa30da8...4f24515. Read the comment docs.

@vince0656 vince0656 marked this pull request as ready for review December 12, 2019 20:16
Copy link

@sirlupinwatson sirlupinwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sirlupinwatson
Copy link

Great Job ALL

Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs testing.. and also to be DRY'd up

app/assets/v2/js/grants/detail.js Outdated Show resolved Hide resolved
app/assets/v2/js/grants/new.js Outdated Show resolved Hide resolved
app/grants/models.py Outdated Show resolved Hide resolved
@thelostone-mc
Copy link
Member

@vince0656
In live right now when you click on the nav item -> it updates the URL's keyword param and we just do a search for keyword
This is same as the search bar

Could you ensure that search bar works as expected and categories does not
mess up the search works as of now

@vince0656
Copy link
Contributor Author

Ok. Hope to look into these requests at some point today. Thanks guys👍🏼

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • DRY code comments from @owocki need to be addressed
  • need a video recording of how it works + ensure design doesn't break on mobile/ tablet
  • how does the card look when there is no tag for a grant.
  • could you also double check if search (the comment i left above) works as expected

Just giving you an update that I've got #5637 coming in which might give migration conflicts => you would have to recreate the migration file if 5637 get's in earlier

@thelostone-mc
Copy link
Member

Additionally checking out the code + syncing up with scott
@vince0656 Hear me out and tell me what you think

A grant could be associated with a multiple categories.
Take any Dapp -> It could be UI + Wallet + ETH2.0
This means the model would have to be updated ->
to be a comma-separated values

1. New Grant -> select multiple categories -> get stored as comma-separated values
Example: Grant 1 -> ['UI', 'UX', 'DeFi']
2. View Grant Card -> it will show all selected categories (as opposed to just one) aka UI, UX, DeFi
3. Search -> We'd have to a contains match to see if a grant is of a particular category
4. This would have to be made shown in the grant detail page + editable if you are a grant owner
( OUT OF SCOPE but this can come in part 2 which I can bounty it out for you if you're up for it )

thelostone-mc:speech_balloon:  1 day ago
@Scott Moore The bounty you had put up!
Are we sure that a grant can be part of only one category?
The way that PR is setup ->
Grant can be either Mobile / UI-UX / Eth2.0 but not all which I don't think is valid always.
Eg: Matic


Scott Moore  16 hours ago
Regarding the categories, a grant can definitely be part of multiple categories.

Copy link

@sirlupinwatson sirlupinwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@owocki
Copy link
Contributor

owocki commented Dec 16, 2019

this feedback makes sense to me... but it is a slight change in scope.. happy to provision more ETH to this if it helps get it across the finish line

@vince0656
Copy link
Contributor Author

@owocki happy to do the change in scope if you chuck more eth. Cheers

@owocki
Copy link
Contributor

owocki commented Dec 16, 2019 via email

@thelostone-mc
Copy link
Member

@vince0656 feel free to hit me up if you hit a roadblock cause I'd like to get this in by next week if possible
Totally open to breaking it into two chunks if that makes it easier

@vince0656
Copy link
Contributor Author

Hi @owocki and @thelostone-mc. Sorry about the delay in replying, I’ve had various house emergencies to attend to this week but I’ve pretty much sorted them now.

I’ve had a think and think that splitting into two to get it over the line is a good idea. HOwever, I think the first part needs to enable support for multiple categories now but only store 1 category. I.e the django backend has support for multiple but UI only allows selecting one as it currently does. That way, the model doesn’t need messing with second time round. Second time round I can make the UI changes to enable this.

What do you guys think?

Given ETH is going down etc is it fair to ask for another 0.6 ETH for the second bit which also covers some bits in the first part?

@vince0656
Copy link
Contributor Author

image

Category badge does work with a slight tweak - in part 2 it will look good with multiple categories :)

@owocki
Copy link
Contributor

owocki commented Dec 18, 2019

@vince0656 0.6 ETH is fine.. :make_it_so: ! can u have a PR thats ready for merge by next week? gitcoin grants round 4 is very soon so we have a sense of urgency here

@vince0656
Copy link
Contributor Author

@owocki I think that's fair - let's get it done 👍

@vince0656
Copy link
Contributor Author

Fingers crossed these build issues are sorted 🤞

@vince0656
Copy link
Contributor Author

Next need to make a Grant model support a list of categories rather than store a single one as it currently does

@thelostone-mc
Copy link
Member

@vince0656 you can check out the bounty model keywords as a reference cause it's the same thing dashboard/models.py

@owocki
Copy link
Contributor

owocki commented Dec 19, 2019

@vince0656 sounds good! looking fwd

Copy link

@sirlupinwatson sirlupinwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work @vince0656 . 🥇

Copy link

@sirlupinwatson sirlupinwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice Code @vince0656

@thelostone-mc
Copy link
Member

@vince0656 you think we'd be able to get this in next week ?

@vince0656
Copy link
Contributor Author

@thelostone-mc apologies - working on it now

@thelostone-mc
Copy link
Member

@vince0656 would you be able to get this to the finish line ?
It's a lil past it's due date and I don't want this PR to go stale !

@vince0656
Copy link
Contributor Author

@thelostone-mc apologies. I've not been very well but am going to jump back into this

@vince0656
Copy link
Contributor Author

Having to run make fresh # docker-compose down -v; docker-compose up -d --build; as having lots of issues running the app after a pull

@vince0656
Copy link
Contributor Author

ok so creation, update and category display are all working with the many to many field / architectural changes requested by @danlipert. I'm going to wrap this up and hopefully record a video to attach to this PR to show this in action

@vince0656 vince0656 requested a review from octavioamu February 9, 2020 21:41
@vince0656
Copy link
Contributor Author

@thelostone-mc @octavioamu @danlipert @owocki we are ready to go 🚀

@thelostone-mc thelostone-mc merged commit 45989e3 into gitcoinco:master Feb 20, 2020
@owocki
Copy link
Contributor

owocki commented Mar 2, 2020

@vince0656
guys is this live now?

https://twitter.com/MesquitaDaniell/status/1234558877984808965

im seeing several things that dont make sense in prod

  • people can see the edit view for grants they dont own
  • people can't edit their own categories.. or cant' figure it out (for example: what on earth do i do with this? https://bits.owocki.com/L1ugJqQo )
  • people cant figure out how to add categories to their new grants when posting them ( https://bits.owocki.com/z8uXZK4X )

gitcoin grants round 5 is less than 2 weeks away. can we get a fix PR in for this?

@danimesq
Copy link

danimesq commented Mar 2, 2020

That's why my bio says "Since 2013 irritating developers on GitHub, with more than 1000 issues open". It gets good results for benefit of all of us.

I didn't quickly noticed it and only remembered now, but yes, it showed the "Edit" button when I'm not logged (phone). https://gitcoin.co/grants/455/floflis-os

I have a question: why a contract for a grant? I mean, it makes sense but, what and how it does?

@vince0656
Copy link
Contributor Author

@vince0656
guys is this live now?

https://twitter.com/MesquitaDaniell/status/1234558877984808965

im seeing several things that dont make sense in prod

  • people can see the edit view for grants they dont own
  • people can't edit their own categories.. or cant' figure it out (for example: what on earth do i do with this? https://bits.owocki.com/L1ugJqQo )
  • people cant figure out how to add categories to their new grants when posting them ( https://bits.owocki.com/z8uXZK4X )

gitcoin grants round 5 is less than 2 weeks away. can we get a fix PR in for this?

Hey @owocki !

Looks like the code has been deployed in production and there seems to be a disconnect between people's expectation of the feature and how it's been designed to work. I think people are expecting the field to allow them to add custom tags. However, there are only a restricted set of categories people can actually choose.

Regarding the grant mentioned in the tweet, looks like you may be able to create a grant without any categories which is probably not desired.

people seeing edit view for grants they don't own - not sure how this is possible and shouldn't be related to work I've done as I made changes to the edit form and not the functionality that triggers the form.

The categories available depend on the grant type of the grant being created. For example, the categories that one can select for tech grants can be queried by hitting:

https://gitcoin.co/grants/categories?type=tech

for media grants:

https://gitcoin.co/grants/categories?type=media

The free text search field for categories (when creating and editing a grant) operates like the team members one and therefore allows you to search for one of the above categories based on the grant type selected. If it's easier, it could be changed to a drop down.

Once the categories have been added, people can then search by category on the grant explorer page.

Video Demonstration
I have separated this in two parts. Firstly, creation and updating of both types of grants as follows:
https://drive.google.com/open?id=1YeoU0InrJAnDGLYciKBoC8egZ1NA_o7a

and then category filtering on the grant explorer as follows:
https://drive.google.com/open?id=1FHwHSiSgt4AxF4y-YUrYmH_1pKEQCHkt

Let me know how you want to proceed as I thought this was all approved and wrapped up

@vince0656
Copy link
Contributor Author

Actually, a pure drop down will not allow multiple category selection. However, there may be a way to preview all possible options to filter using select2js

@vince0656
Copy link
Contributor Author

@owocki looks like you can do it like this instead: https://select2.org/getting-started/basic-usage#multi-select-boxes-pillbox

This way, people know what they can select and can select multiple

@owocki
Copy link
Contributor

owocki commented Mar 2, 2020

yeah we have select2 on the site already.. id recommend using that instead of the autocomplete

also we should not make the categories look editable when ur not viewing ur own grant. thats confusing

@thelostone-mc thelostone-mc mentioned this pull request Mar 5, 2020
8 tasks
@vince0656
Copy link
Contributor Author

@owocki let me take a look and open up a PR for these quick fixes. Thanks

@owocki
Copy link
Contributor

owocki commented Mar 23, 2020

@thelostone-mc this ok to payout?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants