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(points): Add initial points activity feed saga #5206

Merged
merged 17 commits into from
Apr 15, 2024
Merged

Conversation

jophish
Copy link
Contributor

@jophish jophish commented Apr 4, 2024

Description

For RET-1032. Adds in saga/data fetching for points history. (I also started implementing the bottom sheet itself but left it out here in the interest of size, see videos below. Will get out a PR after this as a fast follow.)

See the related PR for the backend /getHistory endpoint here.

Test plan

Unit and manual tested. See videos below.

Happy path (no error)

points-history-happy-path-2024-04-04_15.43.35.mp4

Error

points-history-error-path-2024-04-04_15.44.12.mp4

Related issues

Backwards compatibility

Yes.

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

@jophish jophish changed the title feat(points): Add initial points activity feed feat(points): Add initial points activity feed saga Apr 4, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 85.82%. Comparing base (7d1754e) to head (6c23ea0).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5206      +/-   ##
==========================================
+ Coverage   85.81%   85.82%   +0.01%     
==========================================
  Files         744      747       +3     
  Lines       30455    30521      +66     
  Branches     5274     5283       +9     
==========================================
+ Hits        26134    26195      +61     
- Misses       4082     4087       +5     
  Partials      239      239              
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/points/PointsHome.tsx 100.00% <100.00%> (ø)
src/points/selectors.ts 100.00% <100.00%> (ø)
src/points/types.ts 100.00% <ø> (ø)
src/redux/migrations.ts 96.99% <100.00%> (+<0.01%) ⬆️
src/redux/reducers.ts 94.73% <100.00%> (+0.14%) ⬆️
src/redux/store.ts 78.68% <ø> (ø)
src/web3/networkConfig.ts 100.00% <100.00%> (ø)
test/schemas.ts 90.10% <100.00%> (+0.03%) ⬆️
... and 3 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d1754e...6c23ea0. Read the comment docs.

Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

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

Looking great! it'd be great to work with Kayla to decide if the activity history should be persisted between points homepage visits and between app sessions.

if we should clear the history every time the user comes into the bottom sheet, i think we should consider whether redux and saga is the most straightforward approach. naively i think we'd have a less complex solution with useAsyncCallback...

if we should show previously fetched history, we might want to think about how much history is sensible to keep in redux. currently we just keep putting more stuff there, but that's not good for memory usage. we could consider something like the homefeed (keep only the first page or X items?)

src/points/slice.ts Outdated Show resolved Hide resolved
src/web3/networkConfig.ts Show resolved Hide resolved
src/points/saga.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@kathaypacific kathaypacific left a comment

Choose a reason for hiding this comment

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

a few nit comments but LGTM! 🚀

src/points/PointsHome.tsx Outdated Show resolved Hide resolved
src/points/saga.ts Show resolved Hide resolved
src/points/saga.ts Show resolved Hide resolved
src/points/slice.ts Outdated Show resolved Hide resolved
src/points/saga.test.ts Outdated Show resolved Hide resolved
src/points/saga.test.ts Outdated Show resolved Hide resolved
src/points/saga.test.ts Show resolved Hide resolved
Copy link

emerge-tools bot commented Apr 12, 2024

1 build increased size

Name Version Download Change Install Change Approval
Celo (test)
org.celo.mobile.test
1.82.0 (147) 26.9 MB ⬆️ 3.4 kB (0.01%) 63.9 MB ⬆️ 8.2 kB (0.01%) N/A

Celo (test) 1.82.0 (147)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 8.2 kB (0.01%)
Total download size change: ⬆️ 3.4 kB (0.01%)

Largest size changes

Item Install Size Change
main.jsbundle ⬆️ 8.2 kB

🛸 Powered by Emerge Tools

@jophish jophish added this pull request to the merge queue Apr 15, 2024
Merged via the queue into main with commit e0119a7 Apr 15, 2024
16 checks passed
@jophish jophish deleted the jophish/ret-1032 branch April 15, 2024 04:59
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

For
[RET-1032](https://linear.app/valora/issue/RET-1032/[wallet-fake-backend]-points-activity-history).
Adds in saga/data fetching for points history. (I also started
implementing the bottom sheet itself but left it out here in the
interest of size, see videos below. Will get out a PR after this as a
fast follow.)

See the related PR for the backend `/getHistory` endpoint
[here](https://github.com/valora-inc/points-functions/pull/23).

### Test plan

Unit and manual tested. See videos below.

#### Happy path (no error)


https://github.com/valora-inc/wallet/assets/569401/3fd18685-8f27-4b09-b1b9-227eb1fc92de

#### Error


https://github.com/valora-inc/wallet/assets/569401/72838322-f454-4df0-b49d-e838f984d0d2

### Related issues

- Part of
[RET-1032](https://linear.app/valora/issue/RET-1032/[wallet-fake-backend]-points-activity-history).

### Backwards compatibility

Yes.
### Network scalability

If a new NetworkId and/or Network are added in the future, the changes
in this PR will:

- [x] Continue to work without code changes, OR trigger a compilation
error (guaranteeing we find it when a new network is added)

---------

Co-authored-by: Kathy Luo <kathyluo18@gmail.com>
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.

2 participants