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

Feature/Transaction metadata #372

Merged
merged 32 commits into from
Dec 11, 2020
Merged

Conversation

DeeJayElly
Copy link
Contributor

@DeeJayElly DeeJayElly commented Nov 9, 2020

Feature/Transaction metadata

Screenshots:

Screen Shot 2020-11-22 at 12 36 34 PM

Screen Shot 2020-11-09 at 2 13 00 PM

Screen Shot 2020-11-09 at 2 13 08 PM

Screen Shot 2020-12-10 at 11 29 44 AM

Screen Shot 2020-12-10 at 11 29 35 AM

--

Test Cases

Scenario 1: Transaction with metadata, show
Given I am on Cardano explorer app
And default language is ‘lang’
And I have a transaction with metadata
When I search for the transaction
Then I will see a dialog titled ’title_text’
And I will see the content ‘content_text’
When I select button 'show_text'
Then the dialog will be dismissed
And Metadata will be shown
And Metadata is formatted in readable manner

Example:

lang title_text content_text show_text
en Warning: Unmoderated content Transaction metadata is not moderated, and it may contain inappropriate content. The creator of the transaction provides this information and is not in control of this website's operator. Do you want to see the unmoderated content? Show unmoderated content
jp 警告:非承認制コンテンツ トランザクションメタデータは承認制ではなく、不適切な内容を含む場合があります。この情報はトランザクションの作成者が提供するもので、ウェブサイト管理者の管理下にはありません。管理されていないコンテンツを表示しますか。 管理されていないコンテンツを表示する

Scenario 2: Transaction with metadata, leave
Given I am on Cardano explorer app
And default language is ‘lang’
And I have a transaction with metadata
When I search for the transaction
Then I will see a button 'leave_text'
When I select the button
Then the dialog will be dismissed
And I am back on Cardano App search page

Example:

lang leave_text
en Leave this page
jp このページを離れる

Scenario 3: Transaction without metadata
Given I am on Cardano explorer app
And default language is ‘lang’
And I have a transaction without metadata
When I search for the transaction
Then I will not see a dialog titled ‘title_text’

Example:

lang title_text
en Warning: Unmoderated content
jp 警告:非承認制コンテンツ

Testing Summary

For Hydra build #5131736:

Test cases Results Note
Scenario 1 Pass
Scenario 2 Pass
Scenario 3 Pass
--

@DeeJayElly DeeJayElly added the feature Feature PR label Nov 9, 2020
@DeeJayElly DeeJayElly self-assigned this Nov 9, 2020
@DeeJayElly
Copy link
Contributor Author

@DominikGuzei @rhyslbw This PR is updated with new updates for Metadata feature. Pls review it.

Copy link
Contributor

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

@DeeJayElly
Copy link
Contributor Author

DominikGuzei
DominikGuzei previously approved these changes Nov 26, 2020
@DominikGuzei
Copy link
Contributor

@rhyslbw please check again - I fixed the issues I have found and looks good now 👍

Copy link
Collaborator

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

@DeeJayElly

I'm seeing an error:

Screenshot from 2020-11-30 20-40-00

Some transactions on the mainnet with metadata:

  • 43c7a108ab69847ec2bdc3e283dca65a882271a41f01b19a008043494ff128be
  • f910021138e553c65b96cf3e4647927fcd9f634e06544251f83cffb1891876e8
  • 2532ea3bcb91da8b40785aa6864fc65d022757219c7d406837e50daae475f163

Also, please ensure the warning is only show if the transaction contains metadata

rhyslbw
rhyslbw previously approved these changes Dec 2, 2020
Copy link
Contributor

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

@DeeJayElly @rhyslbw I talked to Darko and he said this:
"For stake pools, we need it once. But for metadata every time."

So I guess we need to change the logic for transactions to show the warning every time (only for tx that have the metadata) … maybe we should make this also less intrusive? Like, showing the warning only when the user actually wants to see the metadata?

@rhyslbw rhyslbw self-requested a review December 7, 2020 10:55
Copy link
Collaborator

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Screenshot from 2020-12-07 21-55-16

@DeeJayElly
Copy link
Contributor Author

@DeeJayElly @rhyslbw I talked to Darko and he said this:
"For stake pools, we need it once. But for metadata every time."

So I guess we need to change the logic for transactions to show the warning every time (only for tx that have the metadata) … maybe we should make this also less intrusive? Like, showing the warning only when the user actually wants to see the metadata?

@DominikGuzei @rhyslbw I have implemented the requested change. Can you guys do another round of review.

Copy link
Collaborator

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

  1. The warning must be shown every time, not once per session
  2. Render the component in place of the metadata, so that the other details are shown

Screenshot from 2020-12-09 12-26-05

@DeeJayElly
Copy link
Contributor Author

  1. The warning must be shown every time, not once per session
  2. Render the component in place of the metadata, so that the other details are shown

Screenshot from 2020-12-09 12-26-05

Fixed

rhyslbw
rhyslbw previously approved these changes Dec 10, 2020
Copy link
Collaborator

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Good result @DeeJayElly 👍

@nikolaglumac
Copy link
Contributor

@rhyslbw please take care of merging and deploying this change 🙏

@miorsufianiohk
Copy link

Hi @DeeJayElly. For Hydra build #5131736. Looks good to me. 👍

Test cases Results Note
Scenario 1 Pass
Scenario 2 Pass
Scenario 3 Pass

@DominikGuzei DominikGuzei merged commit 6c3baa8 into develop Dec 11, 2020
@DominikGuzei DominikGuzei deleted the feature/transaction-metadata branch December 11, 2020 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants