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

Feat/collections #7446

Merged
merged 16 commits into from
Sep 23, 2020
Merged

Feat/collections #7446

merged 16 commits into from
Sep 23, 2020

Conversation

zoek1
Copy link
Contributor

@zoek1 zoek1 commented Sep 16, 2020

@mds1
Copy link
Contributor

mds1 commented Sep 17, 2020

Hey @zoek1, I checked out this branch, merged the latest stable into, did a make fresh, then ran make migrate and got the error below:

CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0084_grantcollections, 0084_auto_20200917_1631 in grants).
To fix them run 'python manage.py makemigrations --merge'
make: *** [migrations] Error 1

@zoek1
Copy link
Contributor Author

zoek1 commented Sep 18, 2020

done @mds1

Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Hey @zoek1, left a few comments below.

I also just did a make fresh but can't seem to load anything afterwards. I ran make migrate but it said there were no migrations to run. Let me know if this is an error on my end or if there's something we need to fix!

image

@@ -46,7 +46,7 @@ class CartData {
return bulk_add_cart;
}

static addToCart(grantData) {
static addToCart(grantData, no_report) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Calls to this function don't pass the second input, so right now the no_report input is never used. This input should either be removed, or added to all calls to addToCart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, i missed this. i used the no_report on bulk operation addAllToCart.

action: 'ADD_ITEM',
metadata: JSON.stringify(cartList)
}, {'X-CSRFToken': $("input[name='csrfmiddlewaretoken']").val()});
if (!no_report) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the above comment, no_report is never used so this will always be undefined right now. Is this the expected behavior? If so some comments explaining this would be useful

});

showSideCart();
_alert(`Congratulations, ${getGrants.grants.length} ${getGrants.grants.length > 1 ? 'grant were' : 'grants was'} added to your cart!`, 'success');
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Should say "grants were" if > 1, and "grants was" if 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@owocki
Copy link
Contributor

owocki commented Sep 21, 2020

looks relaly good!

just pulled this down to test. my feedback for v1.

  1. can we move the 'save collection' button next to 'share cart' in the UI on the checkout page?
  2. is it easy to add a 'featured collections' view?
  3. when there are 70+ items in cart, or in a collection, the site gets very very very slow. (a quick triage of this would be great)

v2:

  1. ability to add/remove grants from collection after the fact
  2. share collection add/remove ability with other team members (so a group of ppl can curate collections together - this can be stubbed in in the backend if it makes it easier.)

@zoek1
Copy link
Contributor Author

zoek1 commented Sep 22, 2020

@owocki i implemented the v1 and v2 requests, could you mind test it again?

@zoek1
Copy link
Contributor Author

zoek1 commented Sep 22, 2020

Hey @zoek1, left a few comments below.

I also just did a make fresh but can't seem to load anything afterwards. I ran make migrate but it said there were no migrations to run. Let me know if this is an error on my end or if there's something we need to fix!

image

I'm not sure what cause this error, as I remember i didn't touch any part of such code 🤔

Also, i added the corresponding migrations

@owocki
Copy link
Contributor

owocki commented Sep 22, 2020

my qa this am:

showstoppers

  1. i see ' cannot fund your own grant' on every grant https://bits.owocki.com/yAubpjeX
  2. did something break about clr eligibility upon add to cart in this PR? all of these grants should be eligible for CLR but it says 'not eligible for clr' in the cart https://bits.owocki.com/E0unD7wA https://bits.owocki.com/2Nuygxdp

minor issues

  1. can we add a 'no data' state for my/featured/ all collections? right now its empty and feels broken https://bits.owocki.com/jkuZzow7
    just say something like "to create a collection, add some grants to cart + click 'save to collection' at checkout"
  2. when i view a collection, id like to have that collection name/desc in the unfurl info (twitter title/desc) pls https://bits.owocki.com/6quedkOg

enhancements

  1. lets brainstorm how we can edit collections on an ongoing basis. this will be important for ppl to be able to ongoing curate their collections. maybe if you have a collection + you click 'save as collection' during checkout, you can be promipted to add those grants to an existing collection instead of to a new one?
  2. having the ability for me to share my ability to curate a collection with someone else would be 💯

open questions

@zoek1
Copy link
Contributor Author

zoek1 commented Sep 22, 2020

@owocki i addressed showstoppers && minor issues

@owocki
Copy link
Contributor

owocki commented Sep 22, 2020

left some comments on zoek via DMs:

  • hmm i see the null state even if there are grants showing up https://bits.owocki.com/4gu9kR7P
  • also the ‘add to collection’ button doesnt work, when i have no cllections. maybe it makes more sense to have the ‘add to collection’ functionality in the “Save as Collection ” button on the cart
  • so instead of having ‘save cart as collection’ it should give you the option to add them to an existing collection instead of saving them https://bits.owocki.com/yAubpnl2
  • also the collections are now showing on the main grant taxonomy

@zoek1
Copy link
Contributor Author

zoek1 commented Sep 23, 2020

  • hmm i see the null state even if there are grants showing up https://bits.owocki.com/4gu9kR7P
  • also the ‘add to collection’ button doesnt work, when i have no cllections.
  • instead of having ‘save cart as collection’ it should give you the option to add them to an existing collection instead of saving them https://bits.owocki.com/yAubpnl2
  • also the collections are now showing on the main grant taxonomy

@owocki

@@ -28,7 +28,7 @@
import twitter
from grants.models import (
CartActivity, CLRMatch, Contribution, Flag, Grant, GrantCategory, GrantCLR, GrantType, MatchPledge, PhantomFunding,
Subscription,
Subscription, GrantCollections,
Copy link
Member

@thelostone-mc thelostone-mc Sep 23, 2020

Choose a reason for hiding this comment

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

^ GrantCollection
Let's keep it singular

class GrantCLRAdmin(admin.ModelAdmin):
list_display = ['pk', 'round_num', 'start_date', 'end_date','is_active']


class GrantCollectionsAdmin(admin.ModelAdmin):
Copy link
Member

Choose a reason for hiding this comment

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

^ GrantCollection

@@ -371,3 +375,5 @@ class GrantCLRAdmin(admin.ModelAdmin):
admin.site.register(GrantType, GrantTypeAdmin)
admin.site.register(GrantCategory, GrantCategoryAdmin)
admin.site.register(GrantCLR, GrantCLRAdmin)
admin.site.register(GrantCollections, GrantCollectionsAdmin)
Copy link
Member

Choose a reason for hiding this comment

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

GrantCollection

)


class GrantCollections(SuperModel):
Copy link
Member

Choose a reason for hiding this comment

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

GrantCollection

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.

4 participants