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

Cart localstorage update #8485

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 74 additions & 5 deletions app/assets/v2/js/cart-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,15 @@ class CartData {
static cartContainsGrantWithId(grantId) {
grantId = String(grantId);
const cart = this.loadCart();
const idList = cart.map(grant => {
return grant.grant_id;
});
const idList = cart.map((grant) => String(grant.grant_id));

return idList.includes(grantId);
}

static share_url(title) {
const checkedOut = this.loadCheckedOut();
const donations = (checkedOut.length > 0 ? checkedOut : this.loadCart());
let bulk_add_cart = 'https://gitcoin.co/grants/cart/bulk-add/';

let bulk_add_cart = `${window.location.host}/grants/cart/bulk-add/`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Member

Choose a reason for hiding this comment

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

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

let network = document.web3network;

if (!network) {
Expand Down Expand Up @@ -53,6 +50,10 @@ class CartData {
}

static addToCart(grantData, no_report) {
// Enforce that grant ID is a string, since currently grant IDs are stored as strings in the cart
grantData.grant_id = String(grantData.grant_id);

// Return if grant is already in cart
if (this.cartContainsGrantWithId(grantData.grant_id)) {
return;
}
Expand Down Expand Up @@ -246,6 +247,74 @@ class CartData {
applyCartMenuStyles();
}

// Updates localStorage with the latest grant data from the backend and returns the new cart
static async updateCart() {
// Read cart data from local storage
const cartData = this.loadCart();

// Make sure no grants have empty currencies, and if so default to 0.001 ETH. This prevents cart
// 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

cartData[index].grant_donation_amount = '0.001';
}
});

// 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(',')}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@octavioamu octavioamu Mar 11, 2021

Choose a reason for hiding this comment

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

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"

const response = await fetch(url);
const latestGrantsData = (await response.json()).grants;

// Update grant info with latest data
cartData.forEach((grant, index) => {
// Find the latestGrantsData entry with the same grant ID as this grant
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Preserve the UUID when updating grant info, or
  2. Update the UUID, or
  3. 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

Copy link
Member

Choose a reason for hiding this comment

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

@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

});

// Save the updated cart data to local storage and return it
this.setCart(cartData);
return cartData;
Comment on lines +273 to +315
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

static loadCheckedOut() {
const checkedOutList = localStorage.getItem('contributions_were_successful');

Expand Down
36 changes: 2 additions & 34 deletions app/assets/v2/js/cart.js
Original file line number Diff line number Diff line change
Expand Up @@ -1322,44 +1322,12 @@ Vue.component('grants-cart', {
token.name = token.symbol;
});

// Read array of grants in cart from localStorage
const grantData = CartData.loadCart();

// Make sure none have empty currencies, and if they do default to 0.001 ETH. This is done
// to prevent the cart from getting stuck loading if a currency is empty
grantData.forEach((grant, index) => {
if (!grant.grant_donation_currency) {
grantData[index].grant_donation_currency = 'ETH';
grantData[index].grant_donation_amount = '0.001';
}
});
CartData.setCart(grantData);
this.grantData = grantData;
// Update then get cart data from localStorage
this.grantData = await CartData.updateCart();

// Initialize array of empty comments
this.comments = this.grantData.map(grant => undefined);

// Get list of all grant IDs and unique tokens in the cart
const grantIds = this.grantData.map(grant => grant.grant_id);

// Fetch updated CLR curves for all grants
const url = `${window.location.origin}/grants/v1/api/grants?pks=${grantIds.join(',')}`;
const response = await fetch(url);
const clrCurves = (await response.json()).grants;

// Update CLR curves
this.grantData.forEach((grant, index) => {
// Find the clrCurves entry with the same grant ID as this grant
const clrIndex = clrCurves.findIndex(item => {
return Number(item.id) === Number(grant.grant_id);
});

// Update grantData from server
this.$set(this.grantData[index], 'grant_clr_prediction_curve', clrCurves[clrIndex].clr_prediction_curve);
this.$set(this.grantData[index], 'is_on_team', clrCurves[clrIndex].is_on_team);

});

// Wait until we can load token list
let elapsedTime = 0;
let delay = 50; // 50 ms debounce
Expand Down
2 changes: 1 addition & 1 deletion app/assets/v2/js/grants/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ if (document.getElementById('grants-showcase')) {
this.bottom = this.scrollEnd();
});
},
mounted() {
async mounted() {
let vm = this;

this.fetchGrants(this.page);
Expand Down
10 changes: 5 additions & 5 deletions app/grants/templates/grants/cart-vue.html
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@
<h1 class="text-left text-black mt-5 mb-3">Grants Cart</h1>

{% comment %} Main container {% endcomment %}
<div v-if="grantData.length === 0" class="font-bigger-1 text-left grant-header-row">
Your cart is empty.
</div>

<div v-else-if="isLoading" class="text-center mb-5">
<div v-if="isLoading" class="text-center mb-5">
<loading-screen class="my-5"></loading-screen>
<div>Fetching cart data...</div>
</div>

<div v-else-if="grantData.length === 0" class="font-bigger-1 text-left grant-header-row">
Your cart is empty.
</div>

<div v-else class="m-auto" style="padding-bottom: 10rem;">
<div>
Expand Down