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

jamesob commented at 3:17 pm on March 29, 2018: member

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

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

selection_016

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.

fanquake added the label Tests on Mar 29, 2018

fanquake added the label GUI on Mar 29, 2018

in src/qt/editaddressdialog.cpp:139 in 9af990a05c outdated

134+    QString bad_addr_purpose = model->purposeForAddress(ui->addressEdit->text());
135+    std::string fmt_str = "The entered address \"%1\" is already in the address book.";
136+
137+    if (bad_addr_purpose == "receive" &&
138+            (mode == NewSendingAddress || mode == EditSendingAddress)) {
139+        fmt_str = "Receiving address \"%1\" cannot be added as a sending address.";

ryanofsky commented at 6:39 pm on March 29, 2018:

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.

jamesob commented at 6:44 pm on March 29, 2018:

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

jamesob force-pushed on Mar 30, 2018

jamesob commented at 2:19 pm on March 30, 2018: member

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

jamesob force-pushed on Mar 30, 2018

promag commented at 6:40 pm on April 1, 2018: member

Concept ACK. Will review later.

jamesob force-pushed on Apr 2, 2018

jamesob commented at 1:46 pm on April 2, 2018: member

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

in src/qt/test/addressbooktests.cpp:48 in a5406962da outdated

43+/**
44+ * Test adding various send addresses to the address book.
45+ *
46+ * There are three cases tested:
47+ *
48+ *   - new_address: a new address would should add as a send address successfully.

jnewbery commented at 3:12 pm on April 2, 2018:

would should

jamesob commented at 3:27 pm on April 2, 2018:

Oops, fixed.

jnewbery commented at 3:23 pm on April 2, 2018: member

Tested ACK a5406962daa18a20e21eda8b578bccfad1630fd4

Looks great. Thanks for the new tests!

jamesob force-pushed on Apr 2, 2018

jamesob commented at 3:30 pm on April 2, 2018: member

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

 0 $ diff -u <(git diff master...a540696) <(git diff master...2d15e9c)
 1
 2--- /proc/self/fd/11    2018-04-02 11:30:04.855218939 -0400
 3+++ /proc/self/fd/12    2018-04-02 11:30:04.855218939 -0400
 4@@ -192,7 +192,7 @@
 5      Mode mode;
 6 diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp
 7 new file mode 100644
 8-index 0000000000..bc9ace14d7
 9+index 0000000000..84c765e48a
10 --- /dev/null
11 +++ b/src/qt/test/addressbooktests.cpp
12 @@ -0,0 +1,140 @@
13@@ -243,7 +243,7 @@
14 + *
15 + * There are three cases tested:
16 + *
17-+ *   - new_address: a new address would should add as a send address successfully.
18++ *   - new_address: a new address which should add as a send address successfully.
19 + *   - existing_s_address: an existing sending address which won't add successfully.
20 + *   - existing_r_address: an existing receiving address which won't add successfully.
21 + *

jnewbery commented at 3:52 pm on April 2, 2018: member

reACK 2d15e9ce7d2ca4cc48ce0010e3c1c227a81f3b78

jamesob force-pushed on Apr 9, 2018

jamesob commented at 9:09 pm on April 9, 2018: member

Rebased, conflicts from the #10244 merge resolved.

in src/interfaces/wallet.h:99 in 4e531b1477 outdated

 94@@ -95,7 +95,8 @@ class Wallet
 95     //! Look up address in wallet, return whether exists.
 96     virtual bool getAddress(const CTxDestination& dest,
 97         std::string* name = nullptr,
 98-        isminetype* is_mine = nullptr) = 0;
 99+        isminetype* is_mine = nullptr,
100+        std::string* purpose = nullptr) = 0;

jnewbery commented at 2:36 pm on April 10, 2018:

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?

jamesob commented at 3:37 pm on April 10, 2018:

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

in src/qt/test/addressbooktests.cpp:109 in 4e531b1477 outdated

104+    auto node = interfaces::MakeNode();
105+    OptionsModel optionsModel(*node);
106+    vpwallets.insert(vpwallets.begin(), &wallet);
107+    WalletModel walletModel(std::move(node->getWallets()[0]), *node, platformStyle.get(), &optionsModel);
108+    vpwallets.erase(vpwallets.begin());
109+    AddressTableModel* addressTableModel = walletModel.getAddressTableModel();

jnewbery commented at 2:37 pm on April 10, 2018:

unused?

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

jamesob commented at 3:37 pm on April 10, 2018:

Good catch, thanks.

in src/Makefile.qttest.include:44 in 4e531b1477 outdated

40@@ -38,11 +41,13 @@ qt_test_test_bitcoin_qt_SOURCES = \
41   qt/test/rpcnestedtests.cpp \
42   qt/test/test_main.cpp \
43   qt/test/uritests.cpp \
44+  qt/test/util.cpp \

jnewbery commented at 2:55 pm on April 10, 2018:

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.

jamesob commented at 3:37 pm on April 10, 2018:

Thanks for spotting this.

jamesob force-pushed on Apr 10, 2018

jamesob commented at 4:36 pm on April 10, 2018: member

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

Reworked getAddress definition and calls to remove use of default params (https://github.com/bitcoin/bitcoin/pull/12830/commits/9db465201d46535a9030d052d858f0e5e2c6e796)
Moved the qt/test/util.cpp makefile inclusion to the right commit (https://github.com/bitcoin/bitcoin/pull/12830/commits/82e8fce0366527fd9f7ab14c32567cf4ef323abd)
Removed the unused addressTableModel

in src/qt/addresstablemodel.h:15 in 2251fdc856 outdated

11@@ -12,6 +12,7 @@ enum class OutputType;
12 
13 class AddressTablePriv;
14 class WalletModel;
15+class CAddressBookData;

jnewbery commented at 5:08 pm on April 10, 2018:

forward declaration unused. Please remove

jamesob commented at 5:48 pm on April 10, 2018:

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

jnewbery commented at 5:10 pm on April 10, 2018: member

Nearly there!

jamesob force-pushed on Apr 10, 2018

in src/qt/editaddressdialog.cpp:147 in 41180ac317 outdated

142+            (mode == NewSendingAddress || mode == EditSendingAddress)) {
143+        fmt_str =
144+            "Address \"%1\" already exists as a receiving address with label "
145+            "\"%2\" and so cannot be added as a sending address.";
146+    }
147+    return tr(fmt_str.c_str()).arg(ui->addressEdit->text()).arg(existing_label);

jonasschnelli commented at 5:52 pm on April 10, 2018:

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)?

jamesob commented at 7:08 pm on April 10, 2018:

Good catch, fixed.

jnewbery commented at 6:12 pm on April 10, 2018: member

utACK 41180ac. Good stuff!

jonasschnelli changes_requested

jamesob force-pushed on Apr 10, 2018

jamesob commented at 7:10 pm on April 10, 2018: member

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 commented at 7:22 pm on April 10, 2018: member

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 commented at 8:20 pm on April 10, 2018: member

@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 force-pushed on Apr 10, 2018

jamesob commented at 8:32 pm on April 10, 2018: member

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

jnewbery commented at 11:31 pm on April 10, 2018: member

utACK e6e1da33783408182e52fa73a11b6559ee0022f7

jonasschnelli approved

jonasschnelli commented at 6:46 am on April 11, 2018: contributor

utACK e6e1da33783408182e52fa73a11b6559ee0022f7

jamesob commented at 1:07 pm on April 16, 2018: member

Any other feedback here?

Add purpose arg to Wallet::getAddress

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

c5b277033a

[qt] Display more helpful message when adding a send address has failed

Addresses #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.

8cdcaee4c7

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

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

9c01be1b85

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

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

5109fc4a9c

jamesob force-pushed on Apr 25, 2018

laanwj commented at 5:18 pm on April 25, 2018: member

utACK 5109fc4a9cb2cbd73c33197fb9129e1413ab051b

laanwj merged this on Apr 25, 2018

laanwj closed this on Apr 25, 2018

laanwj referenced this in commit 25ad2f75f5 on Apr 25, 2018

in src/qt/walletmodel.h:208 in 5109fc4a9c

203@@ -204,6 +204,8 @@ class WalletModel : public QObject
204     QString getWalletName() const;
205 
206     bool isMultiwallet();
207+
208+    AddressTableModel* getAddressTableModel() const { return addressTableModel; }

jimpo commented at 5:35 pm on April 25, 2018:

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).

in src/qt/test/addressbooktests.cpp:63 in 5109fc4a9c

58+    TestChain100Setup test;
59+    CWallet wallet("mock", WalletDatabase::CreateMock());
60+    bool firstRun;
61+    wallet.LoadWallet(firstRun);
62+
63+    auto build_address = [&wallet]() {

jimpo commented at 5:45 pm on April 25, 2018:

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?

in src/qt/test/addressbooktests.cpp:95 in 5109fc4a9c

90+        LOCK(wallet.cs_wallet);
91+        wallet.SetAddressBook(r_key_dest, r_label.toStdString(), "receive");
92+        wallet.SetAddressBook(s_key_dest, s_label.toStdString(), "send");
93+    }
94+
95+    auto check_addbook_size = [&wallet](int expected_size) {

jimpo commented at 5:48 pm on April 25, 2018:

nit: Also could be a regular function.

in src/qt/test/addressbooktests.cpp:103 in 5109fc4a9c

 98+
 99+    // We should start with the two addresses we added earlier and nothing else.
100+    check_addbook_size(2);
101+
102+    // Initialize relevant QT models.
103+    std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));

jimpo commented at 5:49 pm on April 25, 2018:

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.

jimpo commented at 6:02 pm on April 25, 2018: contributor

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.

UdjinM6 referenced this in commit 96cc2f82c2 on May 21, 2021

UdjinM6 referenced this in commit 0ee876f5f2 on May 21, 2021

UdjinM6 referenced this in commit c0c2c32571 on May 21, 2021

UdjinM6 referenced this in commit dddd5c7f06 on May 22, 2021

kittywhiskers referenced this in commit a83681afe5 on May 25, 2021

MarcoFalke locked this on Sep 8, 2021

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