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

(Feature) Proper rejections #2096

Merged
merged 21 commits into from
Apr 7, 2021
Merged

(Feature) Proper rejections #2096

merged 21 commits into from
Apr 7, 2021

Conversation

fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Mar 26, 2021 •
edited
Loading

This PR closes #1652, by:

  • rewording messages:
    • "awaiting" with "needs" for the tx status
    • "cancel" with "reject" for the buttons and concept in general
    • "Cancel/ling transaction" with "on-chain rejection"
  • identifying rejection transactions with a red cross inside a red circle icon
  • identifying queued rejection transaction image
  • removing legacy code in favor of isCancellation flag provided by the client-gateway

@fernandomg
remove isCancelTransaction utility function in favor of `txInfo.isC…
6e76a2a 
…ancellation` flag provided by client-gateway
@fernandomg fernandomg self-assigned this Mar 26, 2021
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@fernandomg fernandomg marked this pull request as draft March 26, 2021 20:19
@github-actions
Copy link

github-actions bot commented Mar 26, 2021 •
edited
Loading

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 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@github-actions
Copy link

@github-actions
Copy link

1 similar comment
@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@fernandomg fernandomg force-pushed the feature/1652-proper-rejection branch from 5e6a4f1 to 8e812e4 Compare March 29, 2021 16:57
@fernandomg fernandomg assigned fernandomg and alongoni and unassigned fernandomg Mar 29, 2021
@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@fernandomg fernandomg marked this pull request as ready for review March 30, 2021 02:13
@fernandomg fernandomg requested a review from dasanra March 30, 2021 02:14
matextrem
matextrem reviewed Mar 31, 2021
View reviewed changes
src/routes/safe/components/Transactions/TxList/TxCollapsed.tsx
@@ -124,7 +127,7 @@ export const TxCollapsed = ({
)

const txCollapsedType = (
<div className={'tx-type' + willBeReplaced}>
<div className={'tx-type' + willBeReplaced + onChainRejection}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use string literal instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I tell you I did it this way because I wanted to avoid the white space at the end? (or adding a trim() call to remove it), will it be enough as a justification? 😬

@francovenica
Copy link
Contributor

Checked the changes in wording
Checked the removal of "Cancelling" wording for "Reject" and variations
Checked the new label On-chain-rejection in the tx type and the "On chain rejection created" at the top of the owners' list who signed.
Checked the wording in the History tab once the tx has been signed

Tried creating tx that were labeled as "Cancellation" because of the "0x" data they had, like Custom Interactions with a explicit "0x" input, and the Wallet Connect Example dummy tx, that was also a empty tx that was always labeled as "cancelling".
Both of those are now properly labeled as "Custom interaction" and "WalletConnect app" type of tx in the Queue and History tables.

An snapshot on how I see it now:
image.png
image.png

@francovenica francovenica self-requested a review April 5, 2021 19:31
@fernandomg
Copy link
Contributor Author

@francovenica, just added a color fix (noticed in your screenshot):
image 👀

@fernandomg
Copy link
Contributor Author

@francovenica, Ok last time I look at your screenshots

image 👀

Nonce label was missing for the nonce === 0

@liliya-soroka
Copy link
Member

@fernandomg , about missed nonce - https://app.zenhub.com/workspaces/safe-multisig---web-5ce554debb310a35b2f8b6f8/issues/gnosis/safe-react/2110 . We had it before.

@francovenica
Copy link
Contributor

The changes look good.

I'll check the #2110 in the regression as well, to see if it is fixed for other tx as well

@francovenica francovenica mentioned this pull request Apr 6, 2021
Closed
@liliya-soroka
Copy link
Member

liliya-soroka commented Apr 6, 2021 •
edited
Loading

@francovenica

  • small note to the color fic for Execute . The same color should be used for circle and Execute text ( if it's not available both should be grey out)
    image

@fernandomg
Copy link
Contributor Author

Thanks @liliya-soroka, fixed the 'Execute' color.

/cc @francovenica

@dasanra dasanra merged commit c96e319 into development Apr 7, 2021
@dasanra dasanra deleted the feature/1652-proper-rejection branch April 7, 2021 10:40
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lukasschor lukasschor lukasschor left review comments

@dasanra dasanra dasanra approved these changes

@matextrem matextrem matextrem approved these changes

@francovenica francovenica francovenica approved these changes

@nicosampler nicosampler nicosampler approved these changes

Assignees

@alongoni alongoni

@fernandomg fernandomg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proper rejections
8 participants
@fernandomg @francovenica @liliya-soroka @nicosampler @matextrem @lukasschor @dasanra @alongoni