BTCPrivate
/
BTCP-Rebase
mirror of https://github.com/BTCPrivate/BTCP-Rebase.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge #12830: [qt] [tests] Clarify address book error messages, 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 https://github.com/bitcoin/bitcoin/issues/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
This commit is contained in:
Wladimir J. van der Laan 2018-04-25 19:15:30 +02:00
parent a785bc3667 5109fc4a9c
commit 25ad2f75f5
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
17 changed files with 279 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/Makefile.qttest.include
View File

@ -12,14 +12,17 @@ TEST_QT_MOC_CPP = \
if ENABLE_WALLET
TEST_QT_MOC_CPP += \
qt/test/moc_addressbooktests.cpp \
qt/test/moc_paymentservertests.cpp \
qt/test/moc_wallettests.cpp
endif
TEST_QT_H = \
qt/test/addressbooktests.h \
qt/test/compattests.h \
qt/test/rpcnestedtests.h \
qt/test/uritests.h \
qt/test/util.h \
qt/test/paymentrequestdata.h \
qt/test/paymentservertests.h \
qt/test/wallettests.h
@ -38,11 +41,13 @@ qt_test_test_bitcoin_qt_SOURCES = \
qt/test/rpcnestedtests.cpp \
qt/test/test_main.cpp \
qt/test/uritests.cpp \
qt/test/util.cpp \
$(TEST_QT_H) \
$(TEST_BITCOIN_CPP) \
$(TEST_BITCOIN_H)
if ENABLE_WALLET
qt_test_test_bitcoin_qt_SOURCES += \
qt/test/addressbooktests.cpp \
qt/test/paymentservertests.cpp \
qt/test/wallettests.cpp \
wallet/test/wallet_test_fixture.cpp

8
src/interfaces/wallet.cpp
View File

@ -152,7 +152,10 @@ public:
{
return m_wallet.DelAddressBook(dest);
}
bool getAddress(const CTxDestination& dest, std::string* name, isminetype* is_mine) override
bool getAddress(const CTxDestination& dest,
std::string* name,
isminetype* is_mine,
std::string* purpose) override
{
LOCK(m_wallet.cs_wallet);
auto it = m_wallet.mapAddressBook.find(dest);
@ -165,6 +168,9 @@ public:
if (is_mine) {
*is_mine = IsMine(m_wallet, dest);
}
if (purpose) {
*purpose = it->second.purpose;
}
return true;
}
std::vector<WalletAddress> getAddresses() override

5
src/interfaces/wallet.h
View File

@ -99,8 +99,9 @@ public:
//! Look up address in wallet, return whether exists.
virtual bool getAddress(const CTxDestination& dest,
std::string* name = nullptr,
isminetype* is_mine = nullptr) = 0;
std::string* name,
isminetype* is_mine,
std::string* purpose) = 0;
//! Get wallet address list.
virtual std::vector<WalletAddress> getAddresses() = 0;

2
src/qt/addressbookpage.h
View File

@ -38,7 +38,7 @@ public:
ForEditing /**< Open address book for editing */
};
explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent);
explicit AddressBookPage(const PlatformStyle *platformStyle, Mode mode, Tabs tab, QWidget *parent = 0);
~AddressBookPage();
void setModel(AddressTableModel *model);

34
src/qt/addresstablemodel.cpp
View File

@ -266,7 +266,8 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
}
// Check for duplicate addresses to prevent accidental deletion of addresses, if you try
// to paste an existing address over another address (with a different label)
if (walletModel->wallet().getAddress(newAddress))
if (walletModel->wallet().getAddress(
newAddress, /* name= */ nullptr, /* is_mine= */ nullptr, /* purpose= */ nullptr))
{
editStatus = DUPLICATE_ADDRESS;
return false;
@ -351,7 +352,8 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
}
// Check for duplicate addresses
{
if(walletModel->wallet().getAddress(DecodeDestination(strAddress)))
if (walletModel->wallet().getAddress(
DecodeDestination(strAddress), /* name= */ nullptr, /* is_mine= */ nullptr, /* purpose= */ nullptr))
{
editStatus = DUPLICATE_ADDRESS;
return QString();
@ -405,21 +407,31 @@ bool AddressTableModel::removeRows(int row, int count, const QModelIndex &parent
return true;
}
/* Look up label for address in address book, if not found return empty string.
*/
QString AddressTableModel::labelForAddress(const QString &address) const
{
{
CTxDestination destination = DecodeDestination(address.toStdString());
std::string name;
if (walletModel->wallet().getAddress(destination, &name))
{
return QString::fromStdString(name);
}
std::string name;
if (getAddressData(address, &name, /* purpose= */ nullptr)) {
return QString::fromStdString(name);
}
return QString();
}
QString AddressTableModel::purposeForAddress(const QString &address) const
{
std::string purpose;
if (getAddressData(address, /* name= */ nullptr, &purpose)) {
return QString::fromStdString(purpose);
}
return QString();
}
bool AddressTableModel::getAddressData(const QString &address,
std::string* name,
std::string* purpose) const {
CTxDestination destination = DecodeDestination(address.toStdString());
return walletModel->wallet().getAddress(destination, name, /* is_mine= */ nullptr, purpose);
}
int AddressTableModel::lookupAddress(const QString &address) const
{
QModelIndexList lst = match(index(0, Address, QModelIndex()),

9
src/qt/addresstablemodel.h
View File

@ -67,10 +67,12 @@ public:
*/
QString addRow(const QString &type, const QString &label, const QString &address, const OutputType address_type);
/* Look up label for address in address book, if not found return empty string.
*/
/** Look up label for address in address book, if not found return empty string. */
QString labelForAddress(const QString &address) const;
/** Look up purpose for address in address book, if not found return empty string. */
QString purposeForAddress(const QString &address) const;
/* Look up row index of an address in the model.
Return -1 if not found.
*/
@ -86,6 +88,9 @@ private:
QStringList columns;
EditStatus editStatus;
/** Look up address book data given an address string. */
bool getAddressData(const QString &address, std::string* name, std::string* purpose) const;
/** Notify listeners that data changed. */
void emitDataChanged(int index);

21
src/qt/editaddressdialog.cpp
View File

@ -109,7 +109,7 @@ void EditAddressDialog::accept()
break;
case AddressTableModel::DUPLICATE_ADDRESS:
QMessageBox::warning(this, windowTitle(),
tr("The entered address \"%1\" is already in the address book.").arg(ui->addressEdit->text()),
getDuplicateAddressWarning(),
QMessageBox::Ok, QMessageBox::Ok);
break;
case AddressTableModel::WALLET_UNLOCK_FAILURE:
@ -129,6 +129,25 @@ void EditAddressDialog::accept()
QDialog::accept();
}
QString EditAddressDialog::getDuplicateAddressWarning() const
{
QString dup_address = ui->addressEdit->text();
QString existing_label = model->labelForAddress(dup_address);
QString existing_purpose = model->purposeForAddress(dup_address);
if (existing_purpose == "receive" &&
(mode == NewSendingAddress || mode == EditSendingAddress)) {
return tr(
"Address \"%1\" already exists as a receiving address with label "
"\"%2\" and so cannot be added as a sending address."
).arg(dup_address).arg(existing_label);
}
return tr(
"The entered address \"%1\" is already in the address book with "
"label \"%2\"."
).arg(dup_address).arg(existing_label);
}
QString EditAddressDialog::getAddress() const
{
return address;

5
src/qt/editaddressdialog.h
View File

@ -30,7 +30,7 @@ public:
EditSendingAddress
};
explicit EditAddressDialog(Mode mode, QWidget *parent);
explicit EditAddressDialog(Mode mode, QWidget *parent = 0);
~EditAddressDialog();
void setModel(AddressTableModel *model);
@ -45,6 +45,9 @@ public Q_SLOTS:
private:
bool saveCurrentRow();
/** Return a descriptive string when adding an already-existing address fails. */
QString getDuplicateAddressWarning() const;
Ui::EditAddressDialog *ui;
QDataWidgetMapper *mapper;
Mode mode;

143
src/qt/test/addressbooktests.cpp Normal file
View File

@ -0,0 +1,143 @@
#include <qt/test/addressbooktests.h>
#include <qt/test/util.h>
#include <test/test_bitcoin.h>
#include <interfaces/node.h>
#include <qt/addressbookpage.h>
#include <qt/addresstablemodel.h>
#include <qt/editaddressdialog.h>
#include <qt/callback.h>
#include <qt/optionsmodel.h>
#include <qt/platformstyle.h>
#include <qt/qvalidatedlineedit.h>
#include <qt/walletmodel.h>
#include <key.h>
#include <pubkey.h>
#include <key_io.h>
#include <wallet/wallet.h>
#include <QTimer>
#include <QMessageBox>
namespace
{
/**
* Fill the edit address dialog box with data, submit it, and ensure that
* the resulting message meets expectations.
*/
void EditAddressAndSubmit(
EditAddressDialog* dialog,
const QString& label, const QString& address, QString expected_msg)
{
QString warning_text;
dialog->findChild<QLineEdit*>("labelEdit")->setText(label);
dialog->findChild<QValidatedLineEdit*>("addressEdit")->setText(address);
ConfirmMessage(&warning_text, 5);
dialog->accept();
QCOMPARE(warning_text, expected_msg);
}
/**
* Test adding various send addresses to the address book.
*
* There are three cases tested:
*
* - 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.
*
* In each case, verify the resulting state of the address book and optionally
* the warning message presented to the user.
*/
void TestAddAddressesToSendBook()
{
TestChain100Setup test;
CWallet wallet("mock", WalletDatabase::CreateMock());
bool firstRun;
wallet.LoadWallet(firstRun);
auto build_address = [&wallet]() {
CKey key;
key.MakeNewKey(true);
CTxDestination dest(GetDestinationForKey(
key.GetPubKey(), wallet.m_default_address_type));
return std::make_pair(dest, QString::fromStdString(EncodeDestination(dest)));
};
CTxDestination r_key_dest, s_key_dest;
// Add a preexisting "receive" entry in the address book.
QString preexisting_r_address;
QString r_label("already here (r)");
// Add a preexisting "send" entry in the address book.
QString preexisting_s_address;
QString s_label("already here (s)");
// Define a new address (which should add to the address book successfully).
QString new_address;
std::tie(r_key_dest, preexisting_r_address) = build_address();
std::tie(s_key_dest, preexisting_s_address) = build_address();
std::tie(std::ignore, new_address) = build_address();
{
LOCK(wallet.cs_wallet);
wallet.SetAddressBook(r_key_dest, r_label.toStdString(), "receive");
wallet.SetAddressBook(s_key_dest, s_label.toStdString(), "send");
}
auto check_addbook_size = [&wallet](int expected_size) {
QCOMPARE(static_cast<int>(wallet.mapAddressBook.size()), expected_size);
};
// We should start with the two addresses we added earlier and nothing else.
check_addbook_size(2);
// Initialize relevant QT models.
std::unique_ptr<const PlatformStyle> platformStyle(PlatformStyle::instantiate("other"));
auto node = interfaces::MakeNode();
OptionsModel optionsModel(*node);
AddWallet(&wallet);
WalletModel walletModel(std::move(node->getWallets()[0]), *node, platformStyle.get(), &optionsModel);
RemoveWallet(&wallet);
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
editAddressDialog.setModel(walletModel.getAddressTableModel());
EditAddressAndSubmit(
&editAddressDialog, QString("uhoh"), preexisting_r_address,
QString(
"Address \"%1\" already exists as a receiving address with label "
"\"%2\" and so cannot be added as a sending address."
).arg(preexisting_r_address).arg(r_label));
check_addbook_size(2);
EditAddressAndSubmit(
&editAddressDialog, QString("uhoh, different"), preexisting_s_address,
QString(
"The entered address \"%1\" is already in the address book with "
"label \"%2\"."
).arg(preexisting_s_address).arg(s_label));
check_addbook_size(2);
// Submit a new address which should add successfully - we expect the
// warning message to be blank.
EditAddressAndSubmit(
&editAddressDialog, QString("new"), new_address, QString(""));
check_addbook_size(3);
}
} // namespace
void AddressBookTests::addressBookTests()
{
TestAddAddressesToSendBook();
}

15
src/qt/test/addressbooktests.h Normal file
View File

@ -0,0 +1,15 @@
#ifndef BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
#define BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H
#include <QObject>
#include <QTest>
class AddressBookTests : public QObject
{
Q_OBJECT
private Q_SLOTS:
void addressBookTests();
};
#endif // BITCOIN_QT_TEST_ADDRESSBOOKTESTS_H

5
src/qt/test/test_main.cpp
View File

@ -13,6 +13,7 @@
#include <qt/test/compattests.h>
#ifdef ENABLE_WALLET
#include <qt/test/addressbooktests.h>
#include <qt/test/paymentservertests.h>
#include <qt/test/wallettests.h>
#endif
@ -99,6 +100,10 @@ int main(int argc, char *argv[])
if (QTest::qExec(&test5) != 0) {
fInvalid = true;
}
AddressBookTests test6;
if (QTest::qExec(&test6) != 0) {
fInvalid = true;
}
#endif
fs::remove_all(pathTemp);

22
src/qt/test/util.cpp Normal file
View File

@ -0,0 +1,22 @@
#include <qt/callback.h>
#include <QApplication>
#include <QMessageBox>
#include <QTimer>
#include <QString>
#include <QPushButton>
#include <QWidget>
void ConfirmMessage(QString* text, int msec)
{
QTimer::singleShot(msec, makeCallback([text](Callback* callback) {
for (QWidget* widget : QApplication::topLevelWidgets()) {
if (widget->inherits("QMessageBox")) {
QMessageBox* messageBox = qobject_cast<QMessageBox*>(widget);
if (text) *text = messageBox->text();
messageBox->defaultButton()->click();
}
}
delete callback;
}), SLOT(call()));
}

12
src/qt/test/util.h Normal file
View File

@ -0,0 +1,12 @@
#ifndef BITCOIN_QT_TEST_UTIL_H
#define BITCOIN_QT_TEST_UTIL_H
/**
* Press "Ok" button in message box dialog.
*
* @param text - Optionally store dialog text.
* @param msec - Number of miliseconds to pause before triggering the callback.
*/
void ConfirmMessage(QString* text = nullptr, int msec = 0);
#endif // BITCOIN_QT_TEST_UTIL_H

18
src/qt/test/wallettests.cpp
View File

@ -1,4 +1,5 @@
#include <qt/test/wallettests.h>
#include <qt/test/util.h>
#include <interfaces/node.h>
#include <qt/bitcoinamountfield.h>
@ -35,21 +36,6 @@
namespace
{
//! Press "Ok" button in message box dialog.
void ConfirmMessage(QString* text = nullptr)
{
QTimer::singleShot(0, makeCallback([text](Callback* callback) {
for (QWidget* widget : QApplication::topLevelWidgets()) {
if (widget->inherits("QMessageBox")) {
QMessageBox* messageBox = qobject_cast<QMessageBox*>(widget);
if (text) *text = messageBox->text();
messageBox->defaultButton()->click();
}
}
delete callback;
}), SLOT(call()));
}
//! Press "Yes" or "Cancel" buttons in modal send confirmation dialog.
void ConfirmSend(QString* text = nullptr, bool cancel = false)
{
@ -264,7 +250,7 @@ void TestGUI()
QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
}
}
} // namespace
void WalletTests::walletTests()
{

10
src/qt/transactiondesc.cpp
View File

@ -102,7 +102,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
if (IsValidDestination(address)) {
std::string name;
isminetype ismine;
if (wallet.getAddress(address, &name, &ismine))
if (wallet.getAddress(address, &name, &ismine, /* purpose= */ nullptr))
{
strHTML += "<b>" + tr("From") + ":</b> " + tr("unknown") + "<br>";
strHTML += "<b>" + tr("To") + ":</b> ";
@ -128,7 +128,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
strHTML += "<b>" + tr("To") + ":</b> ";
CTxDestination dest = DecodeDestination(strAddress);
std::string name;
if (wallet.getAddress(dest, &name) && !name.empty())
if (wallet.getAddress(
dest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty())
strHTML += GUIUtil::HtmlEscape(name) + " ";
strHTML += GUIUtil::HtmlEscape(strAddress) + "<br>";
}
@ -196,7 +197,8 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
{
strHTML += "<b>" + tr("To") + ":</b> ";
std::string name;
if (wallet.getAddress(address, &name) && !name.empty())
if (wallet.getAddress(
address, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty())
strHTML += GUIUtil::HtmlEscape(name) + " ";
strHTML += GUIUtil::HtmlEscape(EncodeDestination(address));
if(toSelf == ISMINE_SPENDABLE)
@ -319,7 +321,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
if (ExtractDestination(vout.scriptPubKey, address))
{
std::string name;
if (wallet.getAddress(address, &name) && !name.empty())
if (wallet.getAddress(address, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr) && !name.empty())
strHTML += GUIUtil::HtmlEscape(name) + " ";
strHTML += QString::fromStdString(EncodeDestination(address));
}

3
src/qt/walletmodel.cpp
View File

@ -274,7 +274,8 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(WalletModelTransaction &tran
{
// Check if we have a new address or an updated label
std::string name;
if (!m_wallet->getAddress(dest, &name))
if (!m_wallet->getAddress(
dest, &name, /* is_mine= */ nullptr, /* purpose= */ nullptr))
{
m_wallet->setAddressBook(dest, strLabel, "send");
}

2
src/qt/walletmodel.h
View File

@ -204,6 +204,8 @@ public:
QString getWalletName() const;
bool isMultiwallet();
AddressTableModel* getAddressTableModel() const { return addressTableModel; }
private:
std::unique_ptr<interfaces::Wallet> m_wallet;
std::unique_ptr<interfaces::Handler> m_handler_status_changed;