Bugfix for ESR Handling #1067

n13 · 2021-08-16T05:43:11Z

🗃 Github Issue Or Explanation for this PR. (What is it supposed to do and Why is needed)

Following changes:

Handle incoming ESR:// link
Scan QR code that's not a token transfer (any chain action)
handling non-transfer transactions in all cases
Scan QR code that is a token transfer
unifying transactions - there was some duplicate code between send and scanning QR codes
Show success dialog on generic transactions
Renaming some stuff.

✅ Checklist

I have tested all my changes.

🕵️‍♂️ Notes for Code Reviewer

Tested the following cases

Scan QR Code on "receive" - transfer SEEDS transaction
Scan QR code on gratitude transaction
Handle incoming ESR link for transaction while app is running
Handle incoming ESR link when app is not running (cold start)

esr://gmN0S9_Eeqy57zv_9xn9eU3hL_bxCbUs-jptJqsXY3-Jtawgk9sHycTQh0qBNwwZgICRoSCIq3n-2VQGEW-tIitFI0ZGBghggtKKMIECk6CZXqWhpwXS8osUiktSc3ISixTK84uygYYwAAA

Some more minor fixes and improvements

🙈 Screenshots

UPDATED confirm dialog with fixed time

👯‍♀️ Paired with

…ansaction_success

n13 · 2021-08-16T12:27:35Z

...v2/screens/transfer/send/send_confirmation/components/custom_transaction_success_dialog.dart

+                Text('Date1:  '.i18n, style: Theme.of(context).textTheme.subtitle2),
+                const SizedBox(width: 16),
+                Text(
+                  DateFormat('dd MMMM yyyy at HH:mm').format(transaction.timestamp ?? DateTime.now()),


There's a fix for the date format in another PR

gguijarro-c-chwy · 2021-08-16T12:07:30Z

lib/v2/screens/app/interactor/viewmodels/app_state.dart

-  factory AppState.initial(GuardianRecoveryRequestData? showGuardianApproveOrDenyScreen) {
+  factory AppState.initial({
+    GuardianRecoveryRequestData? showGuardianApproveOrDenyScreen,
+    // ScanESRResultData? signingRequest,


commented code needed?

lib/v2/screens/app/interactor/viewmodels/app_state.dart

gguijarro-c-chwy · 2021-08-16T12:09:21Z

...v2/screens/transfer/send/send_confirmation/components/custom_transaction_success_dialog.dart

+  }
+}
+
+class DialogRow extends StatelessWidget {


this should be on its one file.

Agree

Made a separate PR for this here #1070

gguijarro-c-chwy · 2021-08-16T12:10:02Z

...reens/transfer/send/send_confirmation/interactor/viewmodels/send_confirmation_arguments.dart

@@ -2,6 +2,12 @@ class SendConfirmationArguments {
  final String account;
  final String name;
  final Map<String, dynamic> data;
+  final int pops;


what is pops? Why do we need this?

We need pops because the screen is shown from different screens, sometimes we need 2 pop() calls, sometimes 3

gguijarro-c-chwy · 2021-08-16T12:11:36Z

lib/v2/screens/transfer/send/send_confirmation/send_confirmation_screen.dart

              showDialog<void>(
                context: context,
                barrierDismissible: false, // user must tap button
-                builder: (BuildContext buildContext) => SendTransactionSuccessDialog(
-                    onCloseButtonPressed: () {
+                builder: (BuildContext buildContext) => SendTransactionSuccessDialog.fromPageCommand(


oh i see why pop is needed, this is so hacky.
There must be a better way to implement this?
can we navigate to a path in the stack? /homePage

IDK! it was like that before!

lib/v2/screens/app/app.dart

gguijarro-c-chwy · 2021-08-16T12:29:12Z

lib/v2/screens/app/interactor/viewmodels/app_bloc.dart

+  AppBloc(this._deeplinkBloc)
+      : super(AppState.initial(
+          showGuardianApproveOrDenyScreen: _deeplinkBloc.state.guardianRecoveryRequestData,
+          // signingRequest: _deeplinkBloc.state.signingRequest,


commented code

lib/v2/screens/app/interactor/viewmodels/app_state.dart

gguijarro-c-chwy · 2021-08-16T12:29:28Z

lib/v2/screens/app/interactor/viewmodels/app_state.dart

@@ -46,16 +50,21 @@ class AppState extends Equatable {
      hasNotification: hasNotification ?? this.hasNotification,
      showGuardianRecoveryAlert: showGuardianRecoveryAlert ?? this.showGuardianRecoveryAlert,
      showGuardianApproveOrDenyScreen: showGuardianApproveOrDenyScreen,
+      // signingRequest: signingRequest ?? this.signingRequest,


commented code

gguijarro-c-chwy · 2021-08-16T12:30:23Z

...v2/screens/transfer/send/send_confirmation/components/custom_transaction_success_dialog.dart

+import 'package:seeds/v2/constants/app_colors.dart';
+import 'package:seeds/v2/i18n/transfer/transfer.i18n.dart';
+
+class CustomTransactionSuccessDialog extends StatelessWidget {


we have a dialog class we extend from when creating dialogs.

Is this dialog different? Should this extend from our base dialogs?

I did not change this at all - I just made a second one based on TransactionSuccessDialog

I didn't want to change inheritance, or anything about it, since I did not want to break it.

gguijarro-c-chwy · 2021-08-16T12:38:42Z

Could each of these changes have been on its own PR instead of all into one large pr?

gguijarro-c-chwy · 2021-08-16T12:52:03Z

@RaulUrtecho and @gguij004 this PR is changing a lot of files and touching very important areas of the code.
Can you guys review this PR and run it.

n13 · 2021-08-16T14:37:21Z

Addressed all issues - waiting for more reviews

Tested deep links also, works fine because the ESR handler does nothing on non-esr links

RaulUrtecho · 2021-08-16T17:02:01Z

I agree with Gery, the PR is very big, it would be good to do it in parts to better understand how each one was resolved.

gguijarro-c-chwy · 2021-08-16T18:20:59Z

@n13 I am not understanding this PR. and i truly believe this can be split into smaller more manageable prs.

I will not approve this PR and the other guys in the team and not comfortable doing so either.

Lets try to split this into smaller PRs, i am here all day tomorrow to chat about it.

gguij004

Hi Team, I agree 100% this pr can be split. However we can sometimes get carry away and start coding until flow is fix. I did test flow and it works just the code itsefl is hard to test due to the amount of changes. I also find something weird weird about the send animation, it now stop at the appBar, not sure if is related to this pr.

n13 · 2021-08-17T01:16:58Z

Hi Team, I agree 100% this pr can be split. However we can sometimes get carry away and start coding until flow is fix. I did test flow and it works just the code itsefl is hard to test due to the amount of changes. I also find something weird weird about the send animation, it now stop at the appBar, not sure if is related to this pr.

It was stopping at the app bar before too in this situation, that bug was already in there!!

So you can't have it both ways, don't fix anything or the PR gets too big, and complain about bugs that were already there.

gguij004 · 2021-08-17T01:24:32Z

Hi Team, I agree 100% this pr can be split. However we can sometimes get carry away and start coding until flow is fix. I did test flow and it works just the code itsefl is hard to test due to the amount of changes. I also find something weird weird about the send animation, it now stop at the appBar, not sure if is related to this pr.

It was stopping at the app bar before too in this situation, that bug was already in there!!

So you can't have it both ways, don't fix anything or the PR gets too big, and complain about bugs that were already there.

Not a bug, is up to the designer. My bad idk why I was thinking you had it different on another pr

RaulUrtecho

Since Grabriel confirms it works.

n13 · 2021-08-17T01:26:20Z

Hi Team, I agree 100% this pr can be split. However we can sometimes get carry away and start coding until flow is fix. I did test flow and it works just the code itsefl is hard to test due to the amount of changes. I also find something weird weird about the send animation, it now stop at the appBar, not sure if is related to this pr.

It was stopping at the app bar before too in this situation, that bug was already in there!!
So you can't have it both ways, don't fix anything or the PR gets too big, and complain about bugs that were already there.

Not a bug, is up to the designer. My bad idk why I was thinking you had it different on another pr

I think it should go all the way to the top, for sure, but it's a minor issue and unrelated to this PR.

n13 · 2021-08-17T01:29:53Z

@n13 I am not understanding this PR. and i truly believe this can be split into smaller more manageable prs.

I will not approve this PR and the other guys in the team and not comfortable doing so either.

Lets try to split this into smaller PRs, i am here all day tomorrow to chat about it.

I think we unnecessarily delay things here without providing a better product, and leaving our users hanging with this bug.

I also disagree, I don't see a way to split it meaningfully in smaller PRs.

That is, yes, I can see about ... IDK 2 or 3 things that could be moved into their own PRs but the main chunk of code would still be pretty big, I don't think it will make it a lot smaller.

Certainly happy to try and move these things into their own PR

Just thinking that if we split it up like that, we also increase risks of adding new bugs and breaking things again that are already working in this PR.

Let's talk about it when you're online.

n13 · 2021-08-17T13:41:31Z

this PR was too big and messy, split up into 4 PRs and merged. Closing this.

n13 added 30 commits August 14, 2021 13:46

showing correct currency on transaction success

e291f7f

handle incoming signing request

fbc0da9

process incoming ESR request

8e7a085

add handling of signing requests

33f8118

ESR link link handling

a968b5c

fix crashing bug in user search

0d42108

renamed class

b31893a

better printouts

568d23d

named parameters, better param names

e0fa4fa

transactions that are not transfers added

1cbb9d7

TransactionModel is now transfer transaction model

4edefab

add signing request

1ff1474

add signing request event

8205cce

signing request app state added

0dcf731

custom transaction success dialog

934730e

support for transactions that are not transfers

6151586

intermediary checkin

9e842f1

better printout

faa4ca9

Merge branch 'master' into bugfix/send_custom_transaction_success

1c7d80b

amount entry needs decimal point

671018d

Merge branch 'bugfix/amount_entry_decimal' into bugfix/send_custom_tr…

33978e8

…ansaction_success

printout

80c8664

fix bug with quantity (was missing "SEEDS"; support for all tokens

c6bc5e0

add timestamp

b3e6eae

add precision to token, getAssetString method

e3301cd

HUSD has precision 2, formatting

7790b83

formatting

c820dfa

fix compiler warning

fa3b6a1

parse incoming timestamps to DateTime

b7b80f4

remove unneeded import

f576734

n13 added 4 commits August 16, 2021 13:59

remove duplicate code

be2a73a

fix lint error

aec8bbb

fix time zone bug and time display on success diallog

6f61f62

fix error messages

6533794

n13 commented Aug 16, 2021

View reviewed changes

cleanup - remove commented code, printouts

883dafb

gguijarro-c-chwy requested changes Aug 16, 2021

View reviewed changes

n13 added 5 commits August 16, 2021 20:56

remove todo

c9d6731

comment added

2b895cb

add link to printout

cd3536f

revert rename

bd46dc2

added TODO and comments

6b7d23d

RaulUrtecho mentioned this pull request Aug 16, 2021

QR scan: Wrong animation, send forever bug #1061

Closed

gguij004 reviewed Aug 17, 2021

View reviewed changes

RaulUrtecho self-requested a review August 17, 2021 01:23

RaulUrtecho approved these changes Aug 17, 2021

View reviewed changes

gguij004 approved these changes Aug 17, 2021

View reviewed changes

n13 mentioned this pull request Aug 17, 2021

Fix QR receive (ESR fix 1) #1080

Merged

1 task

n13 closed this Aug 17, 2021

n13 mentioned this pull request Nov 3, 2021

Refactoring ESR [1/2]: Data model #1305

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix for ESR Handling #1067

Bugfix for ESR Handling #1067

n13 commented Aug 16, 2021 •

edited

n13 Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

n13 Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

n13 Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

n13 Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

n13 Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

gguijarro-c-chwy Aug 16, 2021

n13 Aug 16, 2021

gguijarro-c-chwy commented Aug 16, 2021

gguijarro-c-chwy commented Aug 16, 2021

n13 commented Aug 16, 2021

RaulUrtecho commented Aug 16, 2021

gguijarro-c-chwy commented Aug 16, 2021

gguij004 left a comment

n13 commented Aug 17, 2021

gguij004 commented Aug 17, 2021

RaulUrtecho left a comment

n13 commented Aug 17, 2021

n13 commented Aug 17, 2021

n13 commented Aug 17, 2021

Bugfix for ESR Handling #1067

Bugfix for ESR Handling #1067

Conversation

n13 commented Aug 16, 2021 • edited

🗃 Github Issue Or Explanation for this PR. (What is it supposed to do and Why is needed)

✅ Checklist

🕵️‍♂️ Notes for Code Reviewer

🙈 Screenshots

👯‍♀️ Paired with

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gguijarro-c-chwy commented Aug 16, 2021

gguijarro-c-chwy commented Aug 16, 2021

n13 commented Aug 16, 2021

RaulUrtecho commented Aug 16, 2021

gguijarro-c-chwy commented Aug 16, 2021

gguij004 left a comment

Choose a reason for hiding this comment

n13 commented Aug 17, 2021

gguij004 commented Aug 17, 2021

RaulUrtecho left a comment

Choose a reason for hiding this comment

n13 commented Aug 17, 2021

n13 commented Aug 17, 2021

n13 commented Aug 17, 2021

n13 commented Aug 16, 2021 •

edited