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

Warn user if deleting entries that are referenced. #1744

Merged

Conversation

Projects
None yet
3 participants
@wgml
Copy link
Contributor

wgml commented Mar 16, 2018

Description

User is warned on deletion of entries referenced by other database entries. No warning is shown if all referencing entries are also being removed.

Motivation and context

See #852.

How has this been tested?

Built on top of develop, that is 2.3.1 with gcc 7.3.0 on 64-bit Linux.

Ran tests, of which testgui failed. I checked logs for sanitizer output and compared with running same test on develop branch. In both cases, summary was:
SUMMARY: AddressSanitizer: 110049 byte(s) leaked in 2162 allocation(s).

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@wgml wgml changed the title Warn user if deleting entries that are referenced. Resolves #852. Warn user if deleting entries that are referenced. Mar 16, 2018

@@ -199,6 +201,30 @@ Group* Database::findGroupRecursive(const Uuid& uuid, Group* group)
return nullptr;
}

namespace {
QList<Entry*> resolveReferencesRecursive(const Uuid& uuid, const Group* group)

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

This function should be a member of the Database class. Also recommend overloading the Database::resolveReferences function with this one accepting a specific group.

};

QList<Entry*> result;
std::copy_if(group->entries().begin(), group->entries().end(), std::back_inserter(result), isReference);

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

Please don't mix Qt and std. Recommend the following function call:
auto result = QtConcurrent::blockingFiltered(group->entries(), isReference);

std::copy_if(group->entries().begin(), group->entries().end(), std::back_inserter(result), isReference);

for (Group *child : group->children())
result += resolveReferencesRecursive(uuid, child);

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

Missing { and }

QList<Entry*> references = m_db->resolveReferences(entry->uuid());
for (auto const& el : selectedEntries)
references.removeAll(el);
referencesMap.insert(entry, references.size());

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

Instead of using a referencesMap and lumping everything into one message below, you should use this same loop to process the delete function for each selected entry. That way a user can choose to delete or not delete a particular selected entry "in the moment" and not have to cancel the whole operation and deselect a particular one. If there are no entries with references, no message should be shown beyond the "Are you sure you want to delete XX entries?"

This comment has been minimized.

Copy link
@wgml

wgml Mar 17, 2018

Author Contributor

What should be the options available for references? Replace, Ignore, Keep original, Cancel? Depending on selected set of options, flow might get complicated as we will need to track underlying dependencies between every field that might be removed.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

Keep it simple: Copy Values, Delete All, Skip

I envision a message of: "{entry title} has {X} references. What would you like to do?"

This comment has been minimized.

Copy link
@wgml

wgml Mar 18, 2018

Author Contributor

Delete all is far from simple in implementation terms.
I went with Replace / Ignore / Skip for now. What do you think?

@@ -530,6 +548,19 @@ void DatabaseWidget::deleteEntries()
}
}

QString DatabaseWidget::referencesMessage(QMap<Entry*, int> const& referencesMap)

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Mar 17, 2018

Member

This function is unnecessary if you follow my implementation suggestion above.

This comment has been minimized.

Copy link
@wgml

wgml Mar 18, 2018

Author Contributor

Agreed.

Show resolved Hide resolved src/gui/DatabaseWidget.cpp Outdated
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 17, 2018

Before you go any further please address my comments. This is become more and more complicated.

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Mar 17, 2018

Actually, I have one question - can there possibly be references to all fields of Entry in such context, or are we fine with taking care of password and username only?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 17, 2018

Title, Username, Password, URL, and Notes can be referenced

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Mar 17, 2018

Then, this will get far more complicated that I expected. 😑

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Mar 24, 2018

I merged with develop branch to incorporate recent changes to prompts.
Currently, PR offers options to Replace/Ingore/Skip - implementing Delete all might be a bit more complicated and I suggest separating it into new PR.
CI seems to still fail because of
/opt/qt53/include/QtGui/qopengl.h:110:12: fatal error: 'GL/gl.h' file not found
but I don't think I am the one responsible.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Apr 3, 2018

Please rebase on develop now that the code format was performed.

@droidmonkey droidmonkey added this to the v2.4.0 milestone Apr 5, 2018

@wgml wgml force-pushed the wgml:852-warn-on-deleting-with-references branch from 8b33c20 to 46d3e90 Apr 7, 2018

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Apr 7, 2018

Done.
I'll do that.
Done, squashed for (my) convenience.

@wgml wgml force-pushed the wgml:852-warn-on-deleting-with-references branch 2 times, most recently from 8b33c20 to 7576f39 Apr 7, 2018

@ckieschnick ckieschnick referenced this pull request Jul 12, 2018

Merged

Sharing groups #2109

@wgml wgml closed this Sep 17, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 17, 2018

Any reason why this was closed?

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Sep 17, 2018

PR seemed to be stalled from my perspective. If it is still a candidate for a merge, feel free to reopen it.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 17, 2018

For future reference, if it's still open, we still want it.

@droidmonkey droidmonkey reopened this Sep 17, 2018

@droidmonkey droidmonkey requested a review from TheZ3ro Sep 19, 2018

Show resolved Hide resolved src/core/Entry.cpp Outdated
Show resolved Hide resolved src/core/Entry.cpp Outdated
Show resolved Hide resolved src/core/Entry.cpp Outdated
Show resolved Hide resolved src/core/Tools.cpp Outdated

@wgml wgml force-pushed the wgml:852-warn-on-deleting-with-references branch from b7c7d14 to 4389128 Nov 14, 2018

@wgml

This comment has been minimized.

Copy link
Contributor Author

wgml commented Nov 14, 2018

I finally rebased that feature on top of develop. Most of the code was in conflict state by now.
I added missing braces, as @TheZ3ro suggested.

Warn user if deleting entries that are referenced. Resolves #852.
On warning, references can be replaced with original values or ignored.
Removal process can be also skipped for each conflicting entry.

@droidmonkey droidmonkey force-pushed the wgml:852-warn-on-deleting-with-references branch from 6251862 to 7f22151 Dec 23, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Dec 23, 2018

I rebased and made some modifications to streamline. My only concern with this PR is there may be a significant performance hit when deleting entries in large databases. There might be some more optimizations to make before merging.

Also, to conform with the new Database class structure, the resolve references functions should be moved into the Group class.

@droidmonkey
Copy link
Member

droidmonkey left a comment

See above about optimization

@droidmonkey droidmonkey merged commit c630214 into keepassxreboot:develop Dec 24, 2018

4 checks passed

CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.