Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Feature: deep-linking a single tx #2974

Merged
merged 35 commits into from
Nov 29, 2021
Merged

Feature: deep-linking a single tx #2974

merged 35 commits into from
Nov 29, 2021

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 8, 2021

What it solves

Resolves #970

How this PR fixes it

A new route has been added /{{shortName}}:{{safeAddress}}/transactions/{{txId}}. A new component <TxSingularDetails> fetches the matching transaction and adds it to the store as the transaction components/hooks are deeply integrated with Redux.

The CGW returned transaction 'details' are converted to a 'summary' by means of a new function makeTxFromDetails() because the frontend Transaction type require a mixture of TransactionSummary and TransactionDetails from the CGW. The latter can be used to create a Transaction.

Key changes:

  • Copy deeplink button added to individual transactions
  • Link has transaction id
  • Deeplink page only has transaction details
  • Tabs do not exist on deeplinked transactions because the title is self-explanatory
  • "Details" breadcrumb
  • Created transactions navigate to the deeplinked transaction
  • Anonymised tracking event .../transactions/TRANSACTION_ID

Other notable changes:

  • The gatewayTransactions reducer was cleaned up.
  • txLocation was removed from selectors.
  • A new prop iconType was also added to SRC's copy button component, so that we can assign a custom icon to it.

Known bugs:

The share button is disabled because of the following, which will be split into further issues.

  • Sole-owned Safes can reject transaction a second time until next transaction list poll
  • Multi-owned Safes must confirm rejections from initially queued transaction

Required in next version:

  • Grouped transactions (should fix multi-owned issue above)

How to test it

  1. Open the either the queued/historical transaction list(s) and expand a transaction.
  2. Copy the new transaction link via the new share button.
  3. Open the link and expect the transaction to be displayed in an expanded accordion, as it would be normally.

Screenshots

image
image
image
image
image

@iamacook iamacook marked this pull request as draft November 8, 2021 16:20
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 3 0
Ignored 3 N/A
  • Result: ✅ success

  • Annotations: 3 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


[warning] react-hooks/exhaustive-deps

verifies the list of dependencies for Hooks like useEffect and similar


Report generated by eslint-plus-action

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

@coveralls
Copy link

coveralls commented Nov 8, 2021

Pull Request Test Coverage Report for Build 1499591952

  • 51 of 193 (26.42%) changed or added relevant lines in 21 files are covered.
  • 84 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.3%) to 32.134%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/safe/components/Transactions/TxList/hooks/usePagedHistoryTransactions.ts 0 1 0.0%
src/routes/safe/components/Transactions/TxList/index.tsx 1 2 50.0%
src/routes/safe/components/Transactions/TxList/ActionModal.tsx 0 2 0.0%
src/routes/safe/components/Transactions/TxList/QueueTxList.tsx 0 2 0.0%
src/routes/safe/components/Transactions/TxList/styled.tsx 2 4 50.0%
src/logic/safe/store/actions/fetchTransactionDetails.ts 1 4 25.0%
src/routes/safe/components/Transactions/TxList/TxShareButton.tsx 1 4 25.0%
src/routes/safe/components/Transactions/TxList/hooks/useActionButtonsHandlers.ts 0 3 0.0%
src/routes/safe/components/Transactions/TxList/TxActionProvider.tsx 0 4 0.0%
src/routes/safe/components/Transactions/TxList/hooks/useTransactionDetails.ts 0 4 0.0%
Files with Coverage Reduction New Missed Lines %
src/logic/safe/store/actions/fetchTransactionDetails.ts 1 17.65%
src/routes/index.tsx 1 0%
src/routes/safe/components/Transactions/TxList/hooks/useActionButtonsHandlers.ts 1 1.96%
src/routes/safe/components/Transactions/TxList/hooks/useTransactionActions.ts 1 4.65%
src/routes/safe/components/Transactions/TxList/QueueTxList.tsx 1 12.9%
src/routes/safe/container/index.tsx 1 0%
src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx 3 77.05%
src/logic/safe/utils/safeVersion.ts 9 75.0%
src/logic/hooks/useEstimateTransactionGas.tsx 11 26.67%
src/routes/safe/components/Transactions/helpers/EditTxParametersForm/index.tsx 14 23.26%
Totals Coverage Status
Change from base Build 1495508268: 0.3%
Covered Lines: 3039
Relevant Lines: 8434

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1499620411

Failed tests:

  • ❌ Safe Apps List Safe Apps List

@iamacook iamacook marked this pull request as ready for review November 12, 2021 10:23
| 'customAvatar'
| 'showCopyBtn'
| 'explorerUrl'
| 'shouldShowShortName' // The ommission of all shortName props here is to avoid type error that will be solved by merging PR #2896
Copy link
Member Author

Choose a reason for hiding this comment

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

Awaiting merge of #2896

Copy link
Collaborator

@dasanra dasanra left a comment

Choose a reason for hiding this comment

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

Good work!

@katspaugh
Copy link
Member

katspaugh commented Nov 15, 2021

Nice! 👏

🐞Some txns aren't loading, e.g. this one:
https://pr2974--safereact.review-safe.gnosisdev.com/app/rin:0xb3b83bf204C458B461de9B0CD2739DB152b4fa5A/transactions/0x3d25bef84fce5f96c58eb8ba5bda9cb3af166e738c649b0c2df7ec91103acb5f
Screenshot 2021-11-15 at 09 24 37

Safe Creation tx doesn't have the button. Any idea why?

Also, could we hide the Queue & History tabs in this view?

@iamacook
Copy link
Member Author

Nice! 👏

🐞Some txns aren't loading, e.g. this one: https://pr2974--safereact.review-safe.gnosisdev.com/app/rin:0xb3b83bf204C458B461de9B0CD2739DB152b4fa5A/transactions/0x3d25bef84fce5f96c58eb8ba5bda9cb3af166e738c649b0c2df7ec91103acb5f Screenshot 2021-11-15 at 09 24 37

Safe Creation tx doesn't have the button. Any idea why?

Also, could we hide the Queue & History tabs in this view?

That's strange because it loads for me. I will look into this later.

image

If the tabs are hidden, how would the user determine whethere it's a historical or queued transaction?

@katspaugh
Copy link
Member

Opened fine for me this time. Randomly clicked on some other txns from my history. Some throw.
Looks like it can be reproduced on any tx, and it happens only occasionally. Race condition?

Screenshot 2021-11-15 at 10 03 46

@iamacook iamacook marked this pull request as draft November 22, 2021 13:23
@iamacook iamacook marked this pull request as ready for review November 22, 2021 13:37
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Looks great!

@iamacook iamacook marked this pull request as draft November 24, 2021 11:13
@iamacook iamacook marked this pull request as ready for review November 24, 2021 13:32
@katspaugh katspaugh changed the title Add transaction details route Feature: deep-linking a single tx Nov 25, 2021
@francovenica
Copy link
Contributor

Any tx takes you to what would be the "deep link view" instead of taking you to the Queue tab. Can this be related to the possible duplicated rejection issue?
#3063

@iamacook
Copy link
Member Author

Do you think that we shouldn't navigate to the transaction after creation? I would not necessarily list this as a new issue because it is a simple thing for me to adjust @katspaugh

@liliya-soroka
Copy link
Member

liliya-soroka commented Nov 26, 2021

@francovenica and @iamacook , we agreed to open transaction details for now after transaction creation. Let's stop the work on this PR for now and do full functional testing. Please, test it fully and post each issue separately to discuss and allow
Johannes prioritize them.
No need to polished it in the version 1 if it's not critical from the PM point of view

@francovenica
Copy link
Contributor

I didn't see anywhere that taking the user to the "deep link" view after a tx was intentional. Still is something that I completely disagree with, is a really weird change that will confuse the user a lot

@iamacook
Copy link
Member Author

I mention it in the PR description @francovenica, "Created transactions navigate to the deeplinked transaction" but we can discuss whether we should remove it in the daily.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate deeplinked rejection possible Grouped transaction deeplinking Share deeplink to transaction
6 participants