[qt] [tests] Clarify address book error messages, add tests #12830

jamesob · 2018-03-29T15:17:29Z

Addresses #12796.

When a user attempts to add to the address book a sending address which is already present as a receiving address, they're presented with a confusing error indicating the address is already present in the book, despite the fact that this row is currently invisible.

This change adds a more specific error message indicating its existence as a receiving address (as discussed in the linked issue).

This change also adds some tests exercising use of the address book via QT. Adding so much test code for such a trivial change may seem weird, but it's my hope that this will make further test-writing for address book usage (and other QT features) more approachable.

ryanofsky · 2018-03-29T18:39:32Z

src/qt/editaddressdialog.cpp

+
+    if (bad_addr_purpose == "receive" &&
+            (mode == NewSendingAddress || mode == EditSendingAddress)) {
+        fmt_str = "Receiving address \"%1\" cannot be added as a sending address.";


Would it be possible (or desirable) to include the label of the address that can't be added along with the raw address string in these error messages? Seems like this would provide more context.

Yeah, that's a good idea. Will add.

jamesob · 2018-03-30T14:19:55Z

Incorporated @ryanofsky's feedback and rebased (diff).

promag · 2018-04-01T18:40:03Z

Concept ACK. Will review later.

jamesob · 2018-04-02T13:46:14Z

Fixed Travis failures (unchecked db_cxx.h include, whitespace) and rebased.

jnewbery

Tested ACK a5406962daa18a20e21eda8b578bccfad1630fd4

Looks great. Thanks for the new tests!

jnewbery · 2018-04-02T15:12:20Z

src/qt/test/addressbooktests.cpp

would should

Oops, fixed.

jamesob · 2018-04-02T15:30:42Z

Thanks for the review, @jnewbery. Typo fixed and rebased.

 $ diff -u <(git diff master...a540696) <(git diff master...2d15e9c)

--- /proc/self/fd/11    2018-04-02 11:30:04.855218939 -0400
+++ /proc/self/fd/12    2018-04-02 11:30:04.855218939 -0400
@@ -192,7 +192,7 @@
      Mode mode;
 diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp
 new file mode 100644
-index 0000000000..bc9ace14d7
+index 0000000000..84c765e48a
 --- /dev/null
 +++ b/src/qt/test/addressbooktests.cpp
 @@ -0,0 +1,140 @@
@@ -243,7 +243,7 @@
 + *
 + * There are three cases tested:
 + *
-+ *   - new_address: a new address would should add as a send address successfully.
++ *   - new_address: a new address which should add as a send address successfully.
 + *   - existing_s_address: an existing sending address which won't add successfully.
 + *   - existing_r_address: an existing receiving address which won't add successfully.
 + *

jnewbery · 2018-04-02T15:52:39Z

reACK 2d15e9ce7d2ca4cc48ce0010e3c1c227a81f3b78

jamesob · 2018-04-09T21:09:43Z

Rebased, conflicts from the #10244 merge resolved.

jnewbery · 2018-04-10T14:36:06Z

src/interfaces/wallet.h

This list of optional arguments slightly scares me. How much diff would it be to remove the defaults and change all the places this is called to pass in nullptr?

There are ~9 replacements this'd entail, so not too bad. Willfix.

jnewbery · 2018-04-10T14:37:16Z

src/qt/test/addressbooktests.cpp

unused?

qt/test/addressbooktests.cpp: In function ‘void {anonymous}::TestAddAddressesToSendBook()’: qt/test/addressbooktests.cpp:109:24: warning: unused variable ‘addressTableModel’ [-Wunused-variable] AddressTableModel* addressTableModel = walletModel.getAddressTableModel(); ^

Good catch, thanks.

jnewbery · 2018-04-10T14:55:37Z

src/Makefile.qttest.include

This line is added in the wrong commit (commit Add tests for address book manipulation via EditAddressDialog instead of commit Introduce qt/test/util with a generalized ConfirmMessage). The intermediate commit fails linking.

Thanks for spotting this.

jamesob · 2018-04-10T16:36:48Z

Rebased and addressed @jnewbery's feedback (thanks!):

Reworked getAddress definition and calls to remove use of default params (9db4652)
Moved the qt/test/util.cpp makefile inclusion to the right commit (82e8fce)
Removed the unused addressTableModel

jnewbery

Nearly there!

jnewbery · 2018-04-10T17:08:23Z

src/qt/addresstablemodel.h


 class AddressTablePriv;
 class WalletModel;
+class CAddressBookData;


forward declaration unused. Please remove

Good catch, thanks. Will spend some time figuring out a sensible clang-tidy config to catch this sort of stuff.

jnewbery · 2018-04-10T18:12:34Z

utACK 41180ac. Good stuff!

jonasschnelli · 2018-04-10T17:52:42Z

src/qt/editaddressdialog.cpp

+            "Address \"%1\" already exists as a receiving address with label "
+            "\"%2\" and so cannot be added as a sending address.";
+    }
+    return tr(fmt_str.c_str()).arg(ui->addressEdit->text()).arg(existing_label);


I guess with this, the new translation strings won't end up in the auto. generated translation source file?
Maybe use the tr().arg() part twice above or use the QT_TR_NOOP() (http://doc.qt.io/qt-5/qtglobal.html#QT_TR_NOOP)?

Good catch, fixed.

jamesob · 2018-04-10T19:10:32Z

Incorporated @jonasschnelli's good feedback about tr() usage. I've also recompiled the EN qt locale with cd src/ && make translate and have squashed that in.

jnewbery · 2018-04-10T19:22:51Z

utACK the change to editaddressdialog.cpp. Not sure about bitcoin_en.ts - is that something that usually gets changed in each commit, or just once before a release?

jamesob · 2018-04-10T20:20:15Z

@jnewbery happy to revert the change to bitcoin_en.ts, but @jonasschnelli seemed to think it'd be best to squash them in. The documentation on translation also seems to suggest that the EN locale file should be changed in tandem with QT strings:

src/qt/locale/bitcoin_en.ts is treated in a special way. It is used as the source for all other translations. Whenever a string in the source code is changed, this file must be updated to reflect those changes.

jamesob · 2018-04-10T20:32:21Z

Reverted EN locale changes per @jonasschnelli's advice on IRC.

jnewbery · 2018-04-10T23:31:10Z

utACK e6e1da33783408182e52fa73a11b6559ee0022f7

jonasschnelli

utACK e6e1da33783408182e52fa73a11b6559ee0022f7

jamesob · 2018-04-16T13:07:18Z

Any other feedback here?

Also make all arguments to getAddress required and document args at call sites.

Addresses bitcoin#12796. When we're unable to add a sending address to the address book because it already exists as a receiving address, display a message indicating as much. This should help avoid confusion about an address supposedly already in the book but which isn't currently visible in the interface.

ConfirmMessage is reused in future tests apart from its single usage here.

…ialog Also modifies corresponding QT code to allow for use within test cases.

laanwj · 2018-04-25T17:18:44Z

utACK 5109fc4

…ests 5109fc4 [tests] [qt] Add tests for address book manipulation via EditAddressDialog (James O'Beirne) 9c01be1 [tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage (James O'Beirne) 8cdcaee [qt] Display more helpful message when adding a send address has failed (James O'Beirne) c5b2770 Add purpose arg to Wallet::getAddress (James O'Beirne) Pull request description: Addresses #12796. When a user attempts to add to the address book a sending address which is already present as a receiving address, they're presented with a confusing error indicating the address is already present in the book, despite the fact that this row is currently invisible. ![selection_011](https://user-images.githubusercontent.com/73197/38096704-8a2948d2-3341-11e8-9632-7d563201f28c.jpg) This change adds a more specific error message indicating its existence as a receiving address (as discussed in the linked issue). ![selection_016](https://user-images.githubusercontent.com/73197/38198467-fa26164e-365a-11e8-8fc5-ddab9caf2fbd.jpg) This change also adds some tests exercising use of the address book via QT. Adding so much test code for such a trivial change may seem weird, but it's my hope that this will make further test-writing for address book usage (and other QT features) more approachable. Tree-SHA512: fbdd5564f7a9a2380bbe437f3378e8d4d5fd9201efff4879b72bc23f2cc1c2eecaf2b811994c25070ee052422e48e47901787c2e62cc584774a997fe6a2a327a

jimpo

In commit [tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage, new files are missing copyright notice headers.

I tried testing manually. The error message when adding a new address works as expected. However, when editing an existing address book entry and changing the address, the behavior is pretty funky. If the address was previously in the address book, it will close the dialog and do nothing, displaying no error. If it is not in the address book, it will edit the entry but also change the label to the address, despite the label in the form not changing.

jimpo · 2018-04-25T17:35:32Z

src/qt/walletmodel.h


    bool isMultiwallet();
+
+    AddressTableModel* getAddressTableModel() const { return addressTableModel; }


commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

This method is already defined on line 139 but without const. I think the non-const variant is actually better because it returns a non-const pointer. (While technically allowed, it feels wrong to me to return a non-const pointer to a member from a const method).

jimpo · 2018-04-25T17:45:44Z

src/qt/test/addressbooktests.cpp

+    bool firstRun;
+    wallet.LoadWallet(firstRun);
+
+    auto build_address = [&wallet]() {


commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

nit: Any particular reason for this to be a lambda expression instead of a regular function in the anonymous namespace?

jimpo · 2018-04-25T17:48:06Z

src/qt/test/addressbooktests.cpp

+        wallet.SetAddressBook(s_key_dest, s_label.toStdString(), "send");
+    }
+
+    auto check_addbook_size = [&wallet](int expected_size) {


nit: Also could be a regular function.

jimpo · 2018-04-25T17:49:58Z

src/qt/test/addressbooktests.cpp

+    check_addbook_size(2);
+
+    // Initialize relevant QT models.
+    std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));


commit: [tests] [qt] Add tests for address book manipulation via EditAddressDialog

nit: camelCase vs snake_case of local variables in this function is inconsistent (r_key_dest vs platformStyle). I think snake_case is preferred for new code.

…, add tests 5109fc4 [tests] [qt] Add tests for address book manipulation via EditAddressDialog (James O'Beirne) 9c01be1 [tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage (James O'Beirne) 8cdcaee [qt] Display more helpful message when adding a send address has failed (James O'Beirne) c5b2770 Add purpose arg to Wallet::getAddress (James O'Beirne) Pull request description: Addresses bitcoin#12796. When a user attempts to add to the address book a sending address which is already present as a receiving address, they're presented with a confusing error indicating the address is already present in the book, despite the fact that this row is currently invisible. ![selection_011](https://user-images.githubusercontent.com/73197/38096704-8a2948d2-3341-11e8-9632-7d563201f28c.jpg) This change adds a more specific error message indicating its existence as a receiving address (as discussed in the linked issue). ![selection_016](https://user-images.githubusercontent.com/73197/38198467-fa26164e-365a-11e8-8fc5-ddab9caf2fbd.jpg) This change also adds some tests exercising use of the address book via QT. Adding so much test code for such a trivial change may seem weird, but it's my hope that this will make further test-writing for address book usage (and other QT features) more approachable. Tree-SHA512: fbdd5564f7a9a2380bbe437f3378e8d4d5fd9201efff4879b72bc23f2cc1c2eecaf2b811994c25070ee052422e48e47901787c2e62cc584774a997fe6a2a327a

fanquake added Tests GUI labels Mar 29, 2018

ryanofsky reviewed Mar 29, 2018

View reviewed changes

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from 9af990a to 97318bc Compare March 30, 2018 14:04

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from 97318bc to af66060 Compare March 30, 2018 18:11

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from af66060 to a540696 Compare April 2, 2018 13:44

jnewbery reviewed Apr 2, 2018

View reviewed changes

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from a540696 to 2d15e9c Compare April 2, 2018 15:27

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch 2 times, most recently from c42cbc1 to 4e531b1 Compare April 9, 2018 20:46

jnewbery reviewed Apr 10, 2018

View reviewed changes

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch 2 times, most recently from ba55832 to 2251fdc Compare April 10, 2018 16:33

jnewbery reviewed Apr 10, 2018

View reviewed changes

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from 2251fdc to 41180ac Compare April 10, 2018 17:47

jonasschnelli suggested changes Apr 10, 2018

View reviewed changes

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from 41180ac to 2dd9606 Compare April 10, 2018 19:07

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from 2dd9606 to e6e1da3 Compare April 10, 2018 20:28

jonasschnelli approved these changes Apr 11, 2018

View reviewed changes

jamesob added 4 commits April 25, 2018 13:08

Add purpose arg to Wallet::getAddress

c5b2770

Also make all arguments to getAddress required and document args at call sites.

[tests] [qt] Introduce qt/test/util with a generalized ConfirmMessage

9c01be1

ConfirmMessage is reused in future tests apart from its single usage here.

[tests] [qt] Add tests for address book manipulation via EditAddressD…

5109fc4

…ialog Also modifies corresponding QT code to allow for use within test cases.

jamesob force-pushed the 2018-03-27-send-recv-addressbook-error branch from e6e1da3 to 5109fc4 Compare April 25, 2018 17:13

laanwj merged commit 5109fc4 into bitcoin:master Apr 25, 2018

jimpo reviewed Apr 25, 2018

View reviewed changes

This was referenced Apr 25, 2018

Refactor: separate wallet from node #10973

Merged

Add BitcoinApplication & RPCConsole tests #11625

Merged

Bushstar mentioned this pull request Apr 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#309

Merged

promag mentioned this pull request Apr 29, 2018

Introduce WalletManager #13034

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		bool isMultiwallet();

		AddressTableModel* getAddressTableModel() const { return addressTableModel; }

[qt] [tests] Clarify address book error messages, add tests #12830

[qt] [tests] Clarify address book error messages, add tests #12830

Uh oh!

Conversation

jamesob commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Mar 30, 2018

Uh oh!

promag commented Apr 1, 2018

Uh oh!

jamesob commented Apr 2, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Apr 2, 2018

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

jamesob commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Apr 10, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Apr 10, 2018

Uh oh!

jnewbery commented Apr 10, 2018

Uh oh!

jamesob commented Apr 10, 2018

Uh oh!

jamesob commented Apr 10, 2018

Uh oh!

jnewbery commented Apr 10, 2018

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Apr 16, 2018

Uh oh!

laanwj commented Apr 25, 2018

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jamesob commented Mar 29, 2018 •

edited

Loading