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

fix: refactor batches fetching #2152

Merged
merged 9 commits into from
Oct 9, 2023
Merged

Conversation

flagrede
Copy link
Collaborator

@flagrede flagrede commented Sep 28, 2023

Description

Closes: regen-network/rnd-dev-team#1800

  • convert addDataToBatches into a hook
  • use pagination for batches everywhere across the app (except for stats on the activity page)
  • merge all batches fetching hook into a single one useFetchPaginatedBatches
  • replace useBatchesWithSupply (without react-query) by useFetchPaginatedBatches (with react-query)
  • add getBatchesByIssuerQuery method

For reviewers:
In order to look for regressions, I've put links to all pages using credit batches.
Let me know if I missed any.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. https://deploy-preview-2152--regen-marketplace.netlify.app/ecocredit-batches/1
  2. https://deploy-preview-2152--regen-marketplace.netlify.app/profile/credit-batches
  3. https://deploy-preview-2152--regen-marketplace.netlify.app/project/C01-001
  4. https://deploy-preview-2152--regen-marketplace.netlify.app/credit-classes/C01
  5. https://deploy-preview-2152--regen-marketplace.netlify.app/stats/activity

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

@netlify
Copy link

netlify bot commented Sep 28, 2023

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 3f7b02b
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/65240731aeb9860008471533
😎 Deploy Preview https://deploy-preview-2152--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

On the stats page, for the number of credits at the top, "-" is getting displayed first instead of the loading skeleton
Otherwise looking good

@flagrede flagrede force-pushed the refactor-1800-add-data-to-batches branch from 42b86ba to 421e018 Compare October 3, 2023 13:29
@flagrede
Copy link
Collaborator Author

flagrede commented Oct 3, 2023

@blushi thanks for catching the loading issue, this is fixed.

@flagrede flagrede requested a review from blushi October 3, 2023 13:29
@blushi
Copy link
Member

blushi commented Oct 4, 2023

It might be worth for @erikalogie to test this so we have another person double checking there's no regression, see testing instructions above, thanks!

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Nice refactoring! It would be great to have @erikalogie to test this as well for any regression but tACK on my side

@flagrede flagrede force-pushed the refactor-1800-add-data-to-batches branch from 69c9218 to c269a6b Compare October 9, 2023 10:02
@erikalogie
Copy link
Collaborator

Is it expected that after you click through a few pages, some of the pages of credit batches are missing the tx hash and/or the credit class?

@flagrede
Copy link
Collaborator Author

flagrede commented Oct 9, 2023

@erikalogie Yes, I think we already had that on dev.

@erikalogie
Copy link
Collaborator

Ok then otherwise I believe it looks ok

@flagrede flagrede force-pushed the refactor-1800-add-data-to-batches branch from c269a6b to 3f7b02b Compare October 9, 2023 13:59
@blushi
Copy link
Member

blushi commented Oct 9, 2023

@erikalogie Yes, I think we already had that on dev.

Yeah I think so too, the missing txs are related to an existing limitation: #2152 (comment) (not reproducable on prod yet because we don't have enough credit batches), but the missing credit class could be worth investigating as part of a separate issue

@flagrede flagrede merged commit 0d8b3de into dev Oct 9, 2023
11 checks passed
@flagrede flagrede deleted the refactor-1800-add-data-to-batches branch October 9, 2023 14:11
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.

Refactor addDataToBatches
3 participants