Display invitee from contact list in home feed, rather than "unknown" #2265

kathaypacific · 2022-03-29T13:44:59Z

Description

Currently the transaction item for an invite sent displays as from "Unknown" because the transaction is going to the escrow contract address instead of a known contact address.

In this PR:

add redux variable to track invitation transaction details. due to the transaction feed being updated regularly, it is nice to keep this persisted redux variable separately so that we don't need to refresh the invite details
if the transaction feed item is a sent invite, get the recipient info from the escrow contract
when displaying the transaction feed item, use the fetched recipient details from the escrow contract

Note: we still rely on the contacts cache to display the invitee name, this cache is only refreshed when the user accesses their contacts (e.g. when on the send screen) - this means the invitee will still display as "Unknown" if the user has reinstalled the app/has not accessed their contacts list yet.

Other changes

N/A

Tested

Manually, unit tests to come

How others should test

Send an invitation, then observe the transaction item on the home feed should display the correct invitee name for the transaction

Related issues

Fixes [Invites v2] Ensure invitee details are correct on inviter's home feed #2215

Backwards compatibility

Yes

codecov · 2022-03-29T14:03:00Z

Codecov Report

Merging #2265 (b91dfb9) into main (204aa52) will increase coverage by 0.04%.
The diff coverage is 91.66%.

❗ Current head b91dfb9 differs from pull request most recent head 78995ce. Consider uploading reports for the commit 78995ce to get more accurate results

@@            Coverage Diff             @@
##             main    #2265      +/-   ##
==========================================
+ Coverage   77.70%   77.75%   +0.04%     
==========================================
  Files         588      588              
  Lines       21452    21484      +32     
  Branches     3944     3953       +9     
==========================================
+ Hits        16670    16705      +35     
+ Misses       4717     4714       -3     
  Partials       65       65

Impacted Files	Coverage Δ
packages/mobile/src/recipients/reducer.ts	`83.33% <ø> (ø)`
packages/mobile/src/redux/store.ts	`83.92% <ø> (ø)`
packages/mobile/src/transactions/reducer.ts	`30.30% <33.33%> (+0.14%)`	⬆️
packages/mobile/src/transactions/saga.ts	`43.22% <95.23%> (+11.26%)`	⬆️
packages/mobile/src/redux/migrations.ts	`98.01% <100.00%> (+0.01%)`	⬆️
packages/mobile/src/transactions/actions.ts	`94.44% <100.00%> (+0.32%)`	⬆️
...kages/mobile/src/transactions/transferFeedUtils.ts	`75.36% <100.00%> (+2.94%)`	⬆️
packages/mobile/test/schemas.ts	`100.00% <100.00%> (ø)`
packages/mobile/src/tokens/utils.ts	`97.05% <0.00%> (+2.94%)`	⬆️

Continue to review full report at Codecov.

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

kathaypacific · 2022-03-29T14:27:38Z

packages/mobile/src/recipients/reducer.ts

@@ -44,7 +44,6 @@ export const recipientsReducer = createReducer(initialState, (builder) => {
        ...initialState,
        ...state,
        ...hydrated,
-        phoneRecipientCache: initialState.phoneRecipientCache,


this was causing the cache to be lost after every app restart which is undesirable for a cache i think...

ah yes the reason is partially in this comment in #37:

Removed recipients from the persist blacklist, because I want to persist valoraRecipientsCache. Previously the only variable in the recipients reducer was the recipientCache (renamed to phoneRecipientCache ), which I guess was blacklisted because we reconstruct it at the start of each app session. Not sure what the impact of this is

Now I guess it broke further because we're not anymore importing contacts during app init.
This change seems good then 👍

jeanregisser

Awesome work! 💯
This regression had been bothering me for a while. Great to see it fixed!!

jeanregisser · 2022-03-31T15:49:34Z

packages/mobile/src/recipients/reducer.ts

@@ -44,7 +44,6 @@ export const recipientsReducer = createReducer(initialState, (builder) => {
        ...initialState,
        ...state,
        ...hydrated,
-        phoneRecipientCache: initialState.phoneRecipientCache,


ah yes the reason is partially in this comment in #37:

Removed recipients from the persist blacklist, because I want to persist valoraRecipientsCache. Previously the only variable in the recipients reducer was the recipientCache (renamed to phoneRecipientCache ), which I guess was blacklisted because we reconstruct it at the start of each app session. Not sure what the impact of this is

Now I guess it broke further because we're not anymore importing contacts during app init.
This change seems good then 👍

jeanregisser · 2022-03-31T16:01:45Z

packages/mobile/src/transactions/feed/TransferFeedItem.test.tsx

+    expectDisplay({
+      getByTestId,
+      expectedTitleSections: [
+        `feedItemEscrowSentTitle, {"context":null,"nameOrNumber":"${contactName}"}`,


nit: it's better to have hardcoded expectations to minimize room for errors:

Suggested change

`feedItemEscrowSentTitle, {"context":null,"nameOrNumber":"${contactName}"}`,

`feedItemEscrowSentTitle, {"context":null,"nameOrNumber":"Kate Bell"}`,

fair enough, I updated!

jeanregisser · 2022-03-31T16:49:46Z

packages/mobile/src/transactions/saga.ts

@@ -220,6 +282,7 @@ function* addRecipientProfiles({ transactions }: NewTransactionsInFeedAction) {
 function* watchNewFeedTransactions() {
  yield takeEvery(Actions.NEW_TRANSACTIONS_IN_FEED, cleanupStandbyTransactionsLegacy)
  yield takeEvery(Actions.UPDATE_TRANSACTIONS, cleanupStandbyTransactions)
+  yield takeLatest(Actions.UPDATE_TRANSACTIONS, getInviteTransactionsDetails)


Why not takeEvery?

hum good question...i was thinking that there shouldn't be multiple UPDATE_TRANSACTIONS actions dispatched in quick succession/in parallel, and if there are then the most recent action would contain the information from previous actions (e.g. the transactions fetched at 10:01am would contain the same transactions fetched at 10:00am) so it's safe to take only the latest action...

but now as i am typing this, i remember that we have paginated transaction feed coming soon so maybe it is safer to do takeEvery after all...

jeanregisser · 2022-03-31T16:52:16Z

packages/mobile/src/transactions/saga.ts

+      !existingInviteTransactions[transaction.transactionHash]
+  )
+
+  if (newInviteTransactions.length > 0) {


nit: it's nice to return early to avoid extra indentation.

Suggested change

if (newInviteTransactions.length > 0) {

if (newInviteTransactions.length <= 0) { return: }

that's true, updated

jeanregisser · 2022-03-31T16:54:04Z

packages/mobile/src/transactions/saga.ts

@@ -82,6 +88,62 @@ function* cleanupStandbyTransactions({ transactions }: UpdateTransactionsAction)
  }
 }

+function* getInviteTransactionDetails(txHash: string, blockNumber: string) {


Nothing to do now, but it seems it could be nice if blockchain-api would get this information for the client directly, right?

yeah i also was thinking about this...but would we then need to query the escrow contract a lot more often? with this client side approach, we only need to query the escrow contract once per invite and we can store the invite details locally for display the next time. but if we want this to be returned in the transaction feed query, would the BE have to fetch this information every time because the api is stateless? maybe @gnardini you have some thoughts on this

…#2265) Currently the transaction item for an invite sent displays as from "Unknown" because the transaction is going to the escrow contract address instead of a known contact address. In this PR: - add redux variable to track invitation transaction details. due to the transaction feed being updated regularly, it is nice to keep this persisted redux variable separately so that we don't need to refresh the invite details - if the transaction feed item is a sent invite, get the recipient info from the escrow contract - when displaying the transaction feed item, use the fetched recipient details from the escrow contract <img width="437" alt="Screenshot 2022-03-29 at 16 24 51" src="https://user-images.githubusercontent.com/20150449/160633963-a8206771-32e6-47b4-865b-d3afb36775cb.png"> Note: we still rely on the contacts cache to display the invitee name, this cache is only refreshed when the user accesses their contacts (e.g. when on the send screen) - this means the invitee will still display as "Unknown" if the user has reinstalled the app/has not accessed their contacts list yet. N/A Manually, unit tests to come Send an invitation, then observe the transaction item on the home feed should display the correct invitee name for the transaction - Fixes #2215 Yes

ValoraQA · 2022-04-20T08:33:13Z

Hey @MuckT, We have verified above task on Latest Android Internal Release build & Latest Test Flight Release build V1.30.1 and updated status in test rails.

Observation:

User is able to see the correct invitee name for the transaction for saved contact.

Test rail link:
iOS15: https://valoraapp.testrail.io/index.php?/tests/view/27232
Android 11: https://valoraapp.testrail.io/index.php?/tests/view/27294
Device: iPhone 13 mini(15.1.1), Google Pixel 2XL(11.0)

CC: @kathaypacific

Thanks..!!

ValoraQA · 2022-04-21T10:24:13Z

Hey @MuckT, We have verified above task on Latest Test Flight Release build V1.30.1, Android Internal Release Build V1.30.1 and updated status in test rails.

User is able to see the correct invitee name for the transaction for saved contact.

Test rail link:
iOS14: https://valoraapp.testrail.io/index.php?/tests/view/27325
Android 9: https://valoraapp.testrail.io/index.php?/tests/view/27263

Devices: iPhone 12(14.7.1), Redmi Note 5 Pro (9.0)

CC: @kathaypacific
Thanks..!!

get recipient info from escrow contract

5a1dbe3

kathaypacific marked this pull request as draft March 29, 2022 13:45

kathaypacific commented Mar 29, 2022

View reviewed changes

valora-bot added 4 commits March 29, 2022 16:29

make invite transaction mapping less brittle

dabd09b

add some tests

4bdcd34

add unit tests

74011fb

Merge branch 'main' into kathy/invite-recipient-homefeed

aa62ce5

kathaypacific marked this pull request as ready for review March 30, 2022 11:53

Merge branch 'main' into kathy/invite-recipient-homefeed

10de00b

jeanregisser approved these changes Mar 31, 2022

View reviewed changes

MuckT and others added 5 commits March 31, 2022 19:06

Merge branch 'main' into kathy/invite-recipient-homefeed

cbead6b

Merge branch 'main' into kathy/invite-recipient-homefeed

c5fcd6b

Merge branch 'main' into kathy/invite-recipient-homefeed

d71c331

tidy up saga and test

ec5afac

use takeevery

b91dfb9

kathaypacific added the automerge Have PR merge automatically when checks pass label Apr 1, 2022

Merge branch 'main' into kathy/invite-recipient-homefeed

78995ce

mergify bot merged commit 906bd35 into main Apr 1, 2022

mergify bot deleted the kathy/invite-recipient-homefeed branch April 1, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display invitee from contact list in home feed, rather than "unknown" #2265

Display invitee from contact list in home feed, rather than "unknown" #2265

kathaypacific commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

kathaypacific Mar 29, 2022

jeanregisser Mar 31, 2022

jeanregisser left a comment •

edited

Loading

jeanregisser Mar 31, 2022

jeanregisser Mar 31, 2022

kathaypacific Apr 1, 2022

jeanregisser Mar 31, 2022

kathaypacific Apr 1, 2022

jeanregisser Mar 31, 2022

kathaypacific Apr 1, 2022

jeanregisser Mar 31, 2022

kathaypacific Apr 1, 2022

ValoraQA commented Apr 20, 2022 •

edited

Loading

ValoraQA commented Apr 21, 2022 •

edited

Loading

	`feedItemEscrowSentTitle, {"context":null,"nameOrNumber":"${contactName}"}`,
	`feedItemEscrowSentTitle, {"context":null,"nameOrNumber":"Kate Bell"}`,

	if (newInviteTransactions.length > 0) {
	if (newInviteTransactions.length <= 0) { return: }

Display invitee from contact list in home feed, rather than "unknown" #2265

Display invitee from contact list in home feed, rather than "unknown" #2265

Conversation

kathaypacific commented Mar 29, 2022 • edited Loading

Description

Other changes

Tested

How others should test

Related issues

Backwards compatibility

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment • 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

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

ValoraQA commented Apr 20, 2022 • edited Loading

ValoraQA commented Apr 21, 2022 • edited Loading

kathaypacific commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

jeanregisser left a comment •

edited

Loading

ValoraQA commented Apr 20, 2022 •

edited

Loading

ValoraQA commented Apr 21, 2022 •

edited

Loading