Cart localstorage update #8485

mds1 · 2021-03-04T21:17:45Z

Local storage format is preserved, but cart.js now updates all grant data on load with a new method CartData.updateCart()
Share cart URL now uses the current domain instead of hardcoding gitcoin.co, so it will now work on localhost/staging
Reorder v-if statements in cart-vue.html to avoid users erroneously seeing "Your cart is empty" when it's actually just loading grant data
Post-checkout modal still works, since local storage format is preserved, but this is not shown in the below video

I also have two questions, which I'll add as comments on the diff

Kapture.2021-03-04.at.13.12.00.mp4

Share cart URLs now use the current domain instead of hardcoding gitcoin.co

mds1 · 2021-03-04T21:19:54Z

app/assets/v2/js/cart-data.js


    return idList.includes(grantId);
  }

  static share_url(title) {
    const donations = this.loadCart();
-    let bulk_add_cart = 'https://gitcoin.co/grants/cart/bulk-add/';
-
+    let bulk_add_cart = `${window.location.host}/grants/cart/bulk-add/`;


Do we want to change anything about the bulk-add endpoint? @octavioamu my understanding was that you wanted to remove it and instead have the Share Cart URL look like https://gitcoin.co/grants/cart?ids=<grantIdsList>. However, I realized one issue with that is the preview links that you see on Twitter would be broken, and would no longer say "3 grants in cart shared by mds1"

Oh i think we should still retain the functionality given the bulk-add endpoint has a few extra logic like being able to add grants using

CLR pk

category pk

subcategory pk

ensuring grants owned by you don't get added

^ This is useful for the Add All to Cart functionality

let's see what @octavioamu was getting at

mds1 · 2021-03-04T21:21:13Z

app/assets/v2/js/cart-data.js

+      //   payment_status:            Updated on-demand in cart.js `updatePaymentStatus()` method
+      //   token_local_id:            Existing localStorage value is preserved
+      //   txnid:                     Updated on-demand in cart.js `updatePaymentStatus()` method
+      //   uuid:                      TODO is this needed or can it be removed? Currently existing value is preserved


What is the UUID used for? what is a grant’s UUID used for? A non-deterministic UUID is generated in CartData.addToCart(), but I don’t see it used anywhere. Found the PR it was introduced in case that helps: #6902

In this section of code, should we:

Preserve the UUID when updating grant info, or

Update the UUID, or

Get rid of the grant UUIDs entirely

Seems 3 is the way to go, but want to make sure I’m not missing anything first

@owocki / @zoek1 would you be able to comment on this ?

I see the reason it was brought up on #6892 but not clear beyond that !

@mds1 -> if we aren't sure and we know currently it isn't updated , I'd lean towards preserving that just to avoid weird side effects. Let's wait to see what kevin / zoek respond

thelostone-mc · 2021-03-05T03:49:24Z

@mds1 quick question !

If I wanted to extend this to also pass the grantIds to this endpoint https://github.com/gitcoinco/web/blob/master/app/grants/urls.py#L88

so that I can access it here

web/app/grants/views.py

Line 2422 in bc0bfbd

def grants_cart_view(request):

how would we go about it ?

reason I ask : I'd want to be able to get the grant id to determine what all tenants are present and load those needed scripts as opposed to loading all the cross chain scripts.
https://github.com/gitcoinco/web/blob/master/app/grants/templates/grants/cart-vue.html#L222-L247

It doesn't make sense for me to load binance scripts if I have only ETH grants on my cart

thelostone-mc · 2021-03-07T09:17:17Z

^ update : a smarter way to go about this would be send the tenants to the URL

gdixon

This is looking great - just 1 quick change needed before we can merge! :) Thanks Matt!!

gdixon · 2021-03-11T02:11:14Z

app/assets/v2/js/cart-data.js

@@ -52,6 +49,10 @@ class CartData {
  }

  static addToCart(grantData, no_report) {
+    // Enforce that grant ID is a number, since backend returns it as a number
+    grantData.grant_id = Number(grantData.grant_id);


This is breaking updateCartItem, removeIdFromCart & cartContainsGrantWithId, could we remove the casting and use Strings instead please? Or make sure that grant_id is ALWAYS a number?

gdixon · 2021-03-11T02:36:43Z

app/assets/v2/js/cart-data.js

@@ -9,19 +9,16 @@ class CartData {
  }

  static cartContainsGrantWithId(grantId) {
-    grantId = String(grantId);
+    grantId = Number(grantId);


Why did we change the casting here?

… currently does

mds1 · 2021-03-11T03:09:55Z

Originally I used Number since the API returns the grant ID as a number. But it seems the default is to use Strings for the grant IDs. I'm not yet sure where the ID is originally casted from a Number to String, but either for simplicity I changed all Number() casts to String() since that will work equally well and should fix the issues you found!

gdixon

Thanks Matt, this is great!! :D

octavioamu

Sorry for my big review, at the moment we are having a big problem of performance on the cart page and I believe this pr will add more delay. I left comments explaining the approach and failing parts but feel free to reach me.

octavioamu · 2021-03-11T20:34:12Z

app/assets/v2/js/grants/index.js

      let vm = this;

+      // Update localStorage with latest cart data. This ensures side cart always shows the correct grant info
+      await CartData.updateCart();


lets remove this one as you only see the sidebar when you add / remove data.
Each time "add to card" is clicked addToCart function is fired, that function fetch the newest data and run add it to CartData.
also there is not information about the clr on the sidebar cart so without this here we avoid a ton of unnecessary requests and screen load time, anyway the data will be refreshed on cart mount when people land into that page

Done in e872850

octavioamu · 2021-03-11T20:37:30Z

app/assets/v2/js/cart-data.js

+    // from being stuck as 'loading' if a currency is empty
+    cartData.forEach((grant, index) => {
+      if (!grant.grant_donation_currency) {
+        cartData[index].grant_donation_currency = 'ETH';


what about others chains that doesn't have eth as token like zcash and others?

Good point—so this was copied from cart.js, which presumably means this was a bug before.

But, do we need this anymore? You may recall we had an issue when we changed cart storage (this issue maybe: #8041) and adding defaults was the fix. So we may be able to just delete this though I'm not 100% sure

ohh ok probably will be handle by the cart tokens logic in that case, but good to make a test with another chain to be sure

octavioamu · 2021-03-11T20:53:18Z

app/assets/v2/js/cart-data.js

+
+    // Get list of all grant IDs and fetch updated grant data from backend
+    const grantIds = cartData.map((grant) => grant.grant_id);
+    const url = `${window.location.origin}/grants/v1/api/grants?pks=${grantIds.join(',')}`;


mmm this will add a lot of data into the json delaying more the cart, extending /cart_payload (used on add to cart) to accept multiple ids will give you only the need data + with the props naming already and then you don't even need to add manually the data to cartData you can use just CartData.addToCart(grant) in the foreach.
That will get into just one place to maintain, short data requests and also handle the tokens on other chains that not ethereum
You can compare with something like
http://localhost:8000/grants/v1/api/grants?pks=35
http://localhost:8000/grants/v1/api/35/cart_payload

metadata on the json call sometimes on prod is huge, also some queries there to populate other related data on the model add a lot of request time.

If you check the diff, you'll notice we were already querying this same endpoint, and we just moved where it happens. So I don't think this will slow anything down compared to the previous baseline?

If so that means this is just an optimization then, and I guess my question is do we want to make optimizations on this PR? If so, that's ok, just want to check

is not the same point not even the same prop names, please check the endpoints on your local with a grant id and you will see the diff just in one grant on local (means without the huge metadata related grants and wall of love) I get a diff of 1.2kb vs 2.7kb so yes will slow a lot.
About already using that endpoint on cart that doesn't mean was a good approach on my opinion and probably is one of the reasons why we are getting complains about cart being so slow, check the prod json the ones more popular have a lot of data, so not sure if we can call my request as "optimization"

octavioamu · 2021-03-11T20:58:54Z

app/assets/v2/js/cart-data.js

+      const grantIndex = latestGrantsData.findIndex((item) => String(item.id) === String(grant.grant_id));
+      const latestGrantData = latestGrantsData[grantIndex];
+
+      // Update with the grant data from server
+      cartData[index].binance_payout_address = latestGrantData.binance_payout_address;
+      cartData[index].celo_payout_address = latestGrantData.celo_payout_address;
+      cartData[index].clr_round_num = latestGrantData.clr_round_num;
+      cartData[index].grant_admin_address = latestGrantData.admin_address;
+      cartData[index].grant_clr_prediction_curve = latestGrantData.clr_prediction_curve;
+      cartData[index].grant_contract_address = latestGrantData.contract_address;
+      cartData[index].grant_contract_version = latestGrantData.contract_version;
+      cartData[index].grant_donation_num_rounds = 1; // always 1, since recurring contributions are not supported
+      cartData[index].grant_id = String(latestGrantData.id); // IDs are saved as strings in localStorage, so we cast to string here
+      cartData[index].grant_image_css = latestGrantData.image_css;
+      cartData[index].grant_logo = latestGrantData.logo_url;
+      cartData[index].grant_slug = latestGrantData.slug;
+      cartData[index].grant_title = latestGrantData.title;
+      cartData[index].grant_token_address = latestGrantData.token_address;
+      cartData[index].grant_token_symbol = latestGrantData.token_symbol;
+      cartData[index].grant_url = latestGrantData.url;
+      cartData[index].harmony_payout_address = latestGrantData.harmony_payout_address;
+      cartData[index].is_clr_eligible = latestGrantData.is_clr_eligible;
+      cartData[index].is_on_team = latestGrantData.is_on_team;
+      cartData[index].kusama_payout_address = latestGrantData.kusama_payout_address;
+      cartData[index].polkadot_payout_address = latestGrantData.polkadot_payout_address;
+      cartData[index].rsk_payout_address = latestGrantData.rsk_payout_address;
+      cartData[index].tenants = latestGrantData.tenants;
+      cartData[index].zcash_payout_address = latestGrantData.zcash_payout_address;
+      cartData[index].zil_payout_address = latestGrantData.zil_payout_address;
+
+      // Other localStorage properties not updated above:
+      //   grant_donation_amount:     Existing localStorage value is preserved
+      //   grant_donation_clr_match:  Updated in cart.js `grantData` watcher
+      //   grant_donation_currency:   Existing localStorage value is preserved
+      //   payment_status:            Updated on-demand in cart.js `updatePaymentStatus()` method
+      //   token_local_id:            Existing localStorage value is preserved
+      //   txnid:                     Updated on-demand in cart.js `updatePaymentStatus()` method
+      //   uuid:                      TODO is this needed or can it be removed? Currently existing value is preserved
+    });
+
+    // Save the updated cart data to local storage and return it
+    this.setCart(cartData);
+    return cartData;


all of this could be replaced by CartData.addToCart(grant) instead so if in the future we need to add / remove data/logic we need just do do it on the addToCart class function.

If we do CartData.addToCart(grant), won't that overwrite a user's grant_donation_amount and grant_donation_currency that are already saved in localstorage? I believe it would, which is why I did it this way as indicated in the comments here

check cart-data.js addToCart it only overwrite if not present

thelostone-mc · 2021-03-17T06:16:04Z

Closing this out as @octavioamu was taking over this task !

mds1 added 2 commits March 4, 2021 09:28

Update cart logic so 'your cart is empty' is not shown too early

ee973d5

Cart page now updates localStorage with latest grant data on load

7edf29a

Share cart URLs now use the current domain instead of hardcoding gitcoin.co

mds1 requested review from chibie, frankchen07, gdixon, octavioamu, PixelantDesign and thelostone-mc as code owners March 4, 2021 21:17

mds1 commented Mar 4, 2021

View reviewed changes

Merge branch 'master' into cart-localstorage-update

7e59e88

gdixon suggested changes Mar 11, 2021

View reviewed changes

gdixon reviewed Mar 11, 2021

View reviewed changes

mds1 added 2 commits March 10, 2021 18:43

Merge branch 'master-upstream' into cart-localstorage-update

64506ab

Use strings for grant IDs in localstorage, since thats what live site…

a8eb425

… currently does

mds1 force-pushed the cart-localstorage-update branch from 3bce14b to a8eb425 Compare March 11, 2021 03:02

gdixon approved these changes Mar 11, 2021

View reviewed changes

octavioamu suggested changes Mar 11, 2021

View reviewed changes

Remove updateCart call in side cart

e872850

thelostone-mc closed this Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cart localstorage update #8485

Cart localstorage update #8485

mds1 commented Mar 4, 2021

mds1 Mar 4, 2021

thelostone-mc Mar 5, 2021

mds1 Mar 4, 2021

thelostone-mc Mar 5, 2021

thelostone-mc commented Mar 5, 2021 •

edited

Loading

thelostone-mc commented Mar 7, 2021

gdixon left a comment

gdixon Mar 11, 2021 •

edited

Loading

gdixon Mar 11, 2021

mds1 commented Mar 11, 2021

gdixon left a comment

octavioamu left a comment

octavioamu Mar 11, 2021

mds1 Mar 11, 2021

octavioamu Mar 11, 2021

mds1 Mar 11, 2021

octavioamu Mar 11, 2021

octavioamu Mar 11, 2021

mds1 Mar 11, 2021

octavioamu Mar 11, 2021 •

edited

Loading

octavioamu Mar 11, 2021

mds1 Mar 11, 2021

octavioamu Mar 11, 2021

thelostone-mc commented Mar 17, 2021

Cart localstorage update #8485

Cart localstorage update #8485

Conversation

mds1 commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelostone-mc commented Mar 5, 2021 • edited Loading

thelostone-mc commented Mar 7, 2021

gdixon left a comment

Choose a reason for hiding this comment

gdixon Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mds1 commented Mar 11, 2021

gdixon left a comment

Choose a reason for hiding this comment

octavioamu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octavioamu Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelostone-mc commented Mar 17, 2021

thelostone-mc commented Mar 5, 2021 •

edited

Loading

gdixon Mar 11, 2021 •

edited

Loading

octavioamu Mar 11, 2021 •

edited

Loading