feat: show list pending offer in Transfers page #441
Conversation
AngelHackAthur
force-pushed
the
arthur/display-transfer-offers-340
branch
from
8ea266a to
942909e
Compare
|
@fayi-da please have a look. |
|
@AngelHackAthur The issue that initiated this work calls for a list of pending transfers (not yet accepted by the transferee) to be added in the Transfers page, below the transfer offer form. The logic is that the transferor is most likely to care about the state of pending transfers:
The solution should also show a status relevant to the transfer, such as "pending acceptance", to make it easy to display a transfer history in the same place in the future, with additional states such as "accepted" and "rejected". |
|
hi @waynecollier-da Just to clarify, you want to display a list of pending transfers (those not yet accepted by the transferee) on the Transfers page, below the transfer offer form, and include relevant statuses like "pending acceptance," "accepted," and "rejected." How is this different from the Transaction History already shown on the Transactions screen? Could you provide more details on how these two features should differ in terms of functionality or user experience? |
|
|
@AngelHackAthur your commit history looks a bit messed up, you have 96 commits in this PR where only the last one actually seems new. Can you please fix it |
AngelHackAthur
force-pushed
the
arthur/display-transfer-offers-340
branch
from
9edeb27 to
607a57c
Compare
|
Hi @waynecollier-da The "pending acceptance" feature has been implemented as follows: |
| const amuletPriceQuery = useAmuletPrice(); | ||
| const primaryPartyId = usePrimaryParty(); | ||
|
|
||
| const toWalletTransferOffer = useCallback( |
There was a problem hiding this comment.
The logic in this function exists in the TransferOffers component. Rather than duplicating it here, we should extract into a util and reuse in both components. I believe the only difference is the predicate for the filter. Can we parameterise that?
|
|
||
| useMemo(() => { | ||
| if (transferOfferContracts && tokenStandardTransferContracts && amuletPrice) { | ||
| const allTransfers: PartialWalletTransferOffer[] = transferOfferContracts |
There was a problem hiding this comment.
Similar to my comment above, the logic for all transfers here is exactly the same as that in TransferOffers. We should extract this into a util and reuse in both components instead.
| return ( | ||
| <Stack spacing={4} direction="column" justifyContent="center" id="transfer-offers"> | ||
| <Typography mt={6} variant="h4"> | ||
| Action Needed{' '} |
There was a problem hiding this comment.
There's no action needed for these items so the header here should say "Pending Offers"
| transferOffer: WalletTransferOffer; | ||
| } | ||
|
|
||
| export const TransferOfferDisplay: React.FC<TransferOfferProps> = props => { |
There was a problem hiding this comment.
Can we rename this to PendingOfferDisplay or something better that explicitly describe what this component is? Same for all the ids and classnames within the component so it's easier to distinguish in tests.
| <Stack direction="row" alignItems="center"> | ||
| <Stack direction="column"> | ||
| <BftAnsEntry | ||
| partyId={offer.senderId} |
There was a problem hiding this comment.
Showing the sender here will not be useful. If Alice is logged in, it'll always say Alice. The recipient will be a more useful Party to display here.
| </Stack> | ||
|
|
||
| <Stack direction="row" alignItems="center" spacing={2}> | ||
| <Button variant="outlined" size="small" className="transfer-offer-accept"> |
There was a problem hiding this comment.
There's no need to have the button here. All items on this list are pending and there's no action to be taken.
There was a problem hiding this comment.
Thanks for the PR. I've left some comments on the code.
For frontend changes, it's very helpful to include a screenshot in the PR to assist with the review process. Please add one after addressing comments.
Also, let’s add some tests for this please. There are some examples in the __tests__ directory of the wallet using vitest.
Let me know if you have any questions or need any clarification!
Please show the recipient, not the sender, in each pending transfer line. The user knows they are the sender; to make it precise you can put that in the header: Pending Offers: Sender alice. But the helpful thing is to know at a glance who hasn't accepted the transfer yet. |
|
Hi @fayi-da Thank you for reviewing! |
Thank you for the clarification! |
|
It's not pretty but the UI meets the base requirement. We'll have to modify it if we get evidence that users want to see their full transfer history for a partyID in this tab/view. I suspect they'll either want that, or they'll want filtering in the transaction history view. I'll let you decide whether to go ahead with this format for now, or push for a cleaner transaction list format rather than separate UI boxes for each transaction. |
There was a problem hiding this comment.
We need a separate test that simulates :
- Alice creates 2 transfer offers to Bob
- Alice logs in and goes to Transfer page
- Alice sees both transfer offers
- Each item in the list renders expected details
If there's a way to add these checks to the existing test, that's fine too.
| </> | ||
| ) : ( | ||
| <Typography variant="pill" className="pending-offer"> | ||
| Pending acceptance |
There was a problem hiding this comment.
We don't need this as it's constant across all items in this list. The title of the section already indicates that each item in the list is pending acknowledgement(acceptance/rejection).
| @@ -110,11 +121,12 @@ export const TransferOffers: React.FC = () => { | |||
| amuletPriceQuery.isError || | |||
| transferOfferContractsQuery.isError || | |||
| tokenStandardTransfersQuery.isError; | |||
| const heading = mode === 'received' ? 'Pending Offers ' : 'Action Needed '; | |||
There was a problem hiding this comment.
We should flip this, see my comment below about the modes.
| @@ -46,7 +52,9 @@ export const TransferOffers: React.FC = () => { | |||
| amuletPrice: BigNumber | |||
| ): Promise<WalletTransferOffer[]> => { | |||
| return items | |||
| .filter(item => item.sender !== primaryPartyId) | |||
| .filter(item => | |||
| mode === 'received' ? item.sender === primaryPartyId : item.sender !== primaryPartyId | |||
There was a problem hiding this comment.
We should flip this, see my comment below about the modes.
| seleniumText( | ||
| offerCard.childElement(className("transfer-offer-received")) | ||
| ) should matchText(expectedAns(bobUserParty, bobAnsName)) | ||
|
|
There was a problem hiding this comment.
Does this pass? You shouldn't see both the sender and receiver at the same time according to the rendering logic below
| @@ -122,6 +122,10 @@ abstract class BaseWalletTransfersFrontendIntegrationTest | |||
| offerCard.childElement(className("transfer-offer-sender")) | |||
| ) should matchText(expectedAns(aliceUserParty, aliceAnsName)) | |||
|
|
|||
| seleniumText( | |||
| offerCard.childElement(className("transfer-offer-received")) | |||
I'm ok with the separate boxes for now. It's low friction and consistent with how we show the other transfer offers. |
|
Hi @fayi-da Sorry for the delay, and thank you for your patience! All the tests are now passing, and I’ve updated the code to match the rendering logic as described Please let me know if there’s anything else to improve 🙏 |
|
Thanks @AngelHackAthur , you're still missing this though:
|
Purpose
Display transfer offers that have not been accepted.
Key Changes
Reference
Closes [#340]