Project

General

Profile

Bug #12375

If a custom-named KeePassX database is found in the Persistent folder, propose renaming it

Added by bertagaz over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
-
Target version:
Start date:
03/19/2017
Due date:
% Done:

100%

Feature Branch:
bugfix/12375-keepassx-prompt-for-moving-database
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Password Manager

Description

While updating the configuration/database of keepassx2, we wanted to cover the case where the user has named its uniq database in ~/Persistent with a different name than the one supported.

We'll add a prompt to the #12369 migration script to ask the user if she wants to move this file to the correct location or not.

keepassx_renaming_prompt.png View (26.6 KB) bertagaz, 03/20/2017 01:07 PM

rephrased.png View (27.2 KB) sajolida, 04/26/2017 03:13 PM


Related issues

Related to Tails - Bug #12368: Design UX for migrating databases to KeePassX 2 Resolved 03/18/2017
Blocked by Tails - Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database Resolved 03/18/2017

Associated revisions

Revision d65da0da (diff)
Added by bertagaz over 2 years ago

Keepassx: move the default database if the user renamed the database.

Refs: #12375

Revision ba49d8b4 (diff)
Added by bertagaz over 2 years ago

Keepassx: DRY, clarify gettext string and fix behavior.

Implement and make it clear that cancelation of the zenity prompt will
open the badly named database file.

Refs: #12375

Revision 5a2da518 (diff)
Added by bertagaz over 2 years ago

Keepassx: add wrapper to the list of files to be looked for translations.

Refs: #12375

Revision 656500d7 (diff)
Added by bertagaz over 2 years ago

Keepassx: really open the user database when told to.

Will-fix: #12375

Revision b3699fc6 (diff)
Added by bertagaz over 2 years ago

Fix KeePassX software name.

Also add better wording when asking for renaming the database file to
the standard name in Tails.

Refs: #12375

Revision 45590968 (diff)
Added by bertagaz over 2 years ago

Use whitespaces indentation everywhere.

Refs: #12375

Revision acd5c16f (diff)
Added by bertagaz over 2 years ago

Fix quoting of gettext strings in KeepassX wrapper.

Refs: #12375

Revision c5b4324e (diff)
Added by bertagaz over 2 years ago

Fix variable quoting.

Refs: #12375

Revision db68910e (diff)
Added by bertagaz over 2 years ago

Simplify test for existence of new database in KeepassX wrapper.

Refs: #12375

Revision 76aef366 (diff)
Added by bertagaz over 2 years ago

Switch tests so that we don't trigger the less robust one in most cases.

Refs: #12375

Revision 3d31afa4 (diff)
Added by bertagaz over 2 years ago

Only check for KeepassX databases if ~/Persitent is a mountpoint.

Refs: #12375

Revision add95a77 (diff)
Added by bertagaz over 2 years ago

KeepassX: Fix gettext syntax breakage brought by last changes.

Will-fix: #12375

Revision 0d165b5f (diff)
Added by bertagaz over 2 years ago

KeepassX: try to fix gettext breakage again.

Refs: #12375

Revision 292e87c7 (diff)
Added by bertagaz over 2 years ago

KeepassX: do not concatenate translated strings.

Refs: #12375

Revision 649bfdc8 (diff)
Added by bertagaz over 2 years ago

KeepassX: use exec to start keepassx.

Refs: #12375

Revision 4d86b6c6
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/bugfix/12375-keepassx-prompt-for-moving-database' into feature/stretch

refs: #12375

Revision 47577dce (diff)
Added by bertagaz over 2 years ago

KeepassX: don't annoy users that don't want to rename their database.

Refs: #12375

Revision 81c64a73 (diff)
Added by bertagaz over 2 years ago

KeepassX: use a better filename to avoid database renaming.

Refs: #12375

Revision 2df84e34 (diff)
Added by bertagaz over 2 years ago

KeepassX: Ensure we run it anyway if the user doesn't want to rename the DB.

Refs: #12375

Revision 3abdaeac (diff)
Added by bertagaz over 2 years ago

KeepassX: Remove one exec call to ensure the old DB is renamed after the migration.

Refs: #12375

Revision 3f82316c
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/bugfix/12375-keepassx-prompt-for-moving-database' into testing (Fix-committed: #12375).

History

#1 Updated by bertagaz over 2 years ago

  • Related to Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database added

#2 Updated by intrigeri over 2 years ago

  • Related to deleted (Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database)

#3 Updated by intrigeri over 2 years ago

  • Blocked by Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database added

#4 Updated by intrigeri over 2 years ago

  • Subject changed from Ask whether to move the keepassx database if not using the supported path to If a custom-named KeePassX database is found in the Persistent folder, propose renaming it

#5 Updated by intrigeri over 2 years ago

  • Priority changed from Normal to Elevated

#6 Updated by bertagaz over 2 years ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from bertagaz to sajolida
  • % Done changed from 0 to 20
  • QA Check set to Info Needed

There are gettext strings in d65da0da27afb2a016d9448e33b8d8ffd6d336e5 that needs to be reviewed UX wise, so reassigning to sajolida.

#7 Updated by bertagaz over 2 years ago

  • Assignee changed from sajolida to bertagaz

bertagaz wrote:

There are gettext strings in d65da0da27afb2a016d9448e33b8d8ffd6d336e5 that needs to be reviewed UX wise, so reassigning to sajolida.

Wait, I'm doing some rephrasing.

#8 Updated by bertagaz over 2 years ago

bertagaz wrote:

Wait, I'm doing some rephrasing.

Ok, the gettext strings in config/chroot_local-includes/usr/local/bin/keepassx are ready for a review UX wise before the code review.

Basically, the user gets prompted with this zenity window, asking whether to rename their new database file when it's not using the default supported filename in Tails. If they click "Rename", the file gets renamed and opened in keepassx, if they click "Open anyway", keepassx open the database without renaming.

Does the wording sounds good?

#9 Updated by intrigeri over 2 years ago

if they click "Open anyway", keepassx open the database without renaming.

… and they'll get the same dialog next time.

#10 Updated by intrigeri over 2 years ago

  • FWIW the thing is called "KeePassX".
  • "seem to" is useless, no?

#11 Updated by bertagaz over 2 years ago

intrigeri wrote:

  • FWIW the thing is called "KeePassX".
  • "seem to" is useless, no?

Right, fixed in b3699fc6e1a3012fb5850307fb86f82e8d67df00

#12 Updated by intrigeri over 2 years ago

  • Assignee changed from sajolida to intrigeri
  • QA Check changed from Info Needed to Ready for QA

I would like to see this usability problem fixed in 3.0~beta4, and doing it with suboptimal strings is better than not doing it at all. I have no clue what's sajolida's ETA on this one, so I'll review the branch, will merge if happy, and will add a note on #12368 about reviewing these strings.

#13 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed
  • Feature Branch set to bugfix/12375-keepassx-prompt-for-moving-database

I did a code review:

  • The message for d65da0da27afb2a016d9448e33b8d8ffd6d336e5 ("move the default database if the user renamed the database") doesn't seem to match what the code does, so I wonder if I get your intent right (the code sounds OK, but when the corresponding description says something else I can't be sure what you're trying to do). Just to clarify, this branch is meant to address the case when a user had an old DB with the default name, and then the first time they run KeePassX on 3.0 it gets imported, but they save it with a name that's not New database. But we don't care about the case when the user's old DB had a non-default name. Correct?
  • Please keep the indentation style consistent: you used tabs in the initial script, and now I see spaces in prompt_for_database_renaming.
  • Please quote variables consistently (this branch introduces some unquoted vars that we quote everywhere else in this script).
  • I don't think we should be looking for a DB in ~/Persistent unless it's a mountpoint.
  • I'm not very happy with the find | wc -l pattern, that is not robust wrt. weirdly named files. I won't block on it though as I have no obvious solution to this in shell (which might hint us that shell was perhaps not the best language for this job). See below for some mitigation measures to limit the impact.
  • When I see [ "$(find $PERSISTENT_DATA_DIR -maxdepth 1 -name 'keepassx.kdbx' 2>/dev/null | wc -l)" = "0" ], I think ! [ -e "$NEW_DB" ]. Any reason not to do the latter?
  • I suggest doing this 2nd test (the one discussed in the previous bullet point) before the find|wc one, so we avoid running this code, that we know lacks robustness, in most cases.

If you can't handle this by May 15, please let me know and I'll take care of it myself.

#14 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

I would like to see this usability problem fixed in 3.0~beta4, and doing it with suboptimal strings is better than not doing it at all. I have no clue what's sajolida's ETA on this one, so I'll review the branch, will merge if happy, and will add a note on #12368 about reviewing these strings.

Agreed. I don't think it's that bad actually anyway.

I did a code review:

Thanks!

  • The message for d65da0da27afb2a016d9448e33b8d8ffd6d336e5 ("move the default database if the user renamed the database") doesn't seem to match what the code does, so I wonder if I get your intent right (the code sounds OK, but when the corresponding description says something else I can't be sure what you're trying to do). Just to clarify, this branch is meant to address the case when a user had an old DB with the default name, and then the first time they run KeePassX on 3.0 it gets imported, but they save it with a name that's not New database. But we don't care about the case when the user's old DB had a non-default name. Correct?

Yes, sorry for the lame commit message...

  • Please keep the indentation style consistent: you used tabs in the initial script, and now I see spaces in prompt_for_database_renaming.

4559096874174e89d941b29588fbabb935b7c165

  • Please quote variables consistently (this branch introduces some unquoted vars that we quote everywhere else in this script).

acd5c16fac8904ab962320901b73a2ff2c11125a

  • I don't think we should be looking for a DB in ~/Persistent unless it's a mountpoint.

3d31afa486917a351ba7ed3b93bd2a80a73af952

  • I'm not very happy with the find | wc -l pattern, that is not robust wrt. weirdly named files. I won't block on it though as I have no obvious solution to this in shell (which might hint us that shell was perhaps not the best language for this job). See below for some mitigation measures to limit the impact.

Did not find another way to do that either.

  • When I see [ "$(find $PERSISTENT_DATA_DIR -maxdepth 1 -name 'keepassx.kdbx' 2>/dev/null | wc -l)" = "0" ], I think ! [ -e "$NEW_DB" ]. Any reason not to do the latter?

Oooch, no idea why I implemented it that way...

db68910ef931753966b62adcf5e8db4efbbc96c4

  • I suggest doing this 2nd test (the one discussed in the previous bullet point) before the find|wc one, so we avoid running this code, that we know lacks robustness, in most cases.

76aef3665c1db18c09fdf72200051c4d7be1c956

#15 Updated by intrigeri over 2 years ago

  • Target version changed from Tails_3.0 to Tails_3.0~rc1

#16 Updated by sajolida over 2 years ago

  • Related to Bug #12368: Design UX for migrating databases to KeePassX 2 added

#17 Updated by sajolida over 2 years ago

Cool! I've rephrased the prompt with 4eaa0ba619. Please have a look and tell me what you think. It also contains tiny bits of bash that I probably go wrong as usual :)

Now my main concern is that if the user chooses to keep the current file name, she will be prompt with this message every time which will very quickly become annoying. Could we avoid this? Maybe dropping an empty dotfile to make that file as seen already (.migration.kdbx.keep in my example). Or would this be overkill?

#18 Updated by sajolida over 2 years ago

The new dialog:

#19 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 20 to 50
  • QA Check changed from Ready for QA to Dev Needed

sajolida's change breaks l10n by concatenating strings: the filename should be a gettext placeholder, and not a string appended to a translated one. Otherwise, looks good to me!

#20 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

sajolida's change breaks l10n by concatenating strings: the filename should be a gettext placeholder, and not a string appended to a translated one. Otherwise, looks good to me!

Fixed in add95a776a2b35b8339308d4a37d05cacaf2a76a. Please review and merge if happy.

#21 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Well, no: we're still concatenating strings, which is never a good idea when l10n matters. So again: the filename should be a gettext placeholder, and not a string appended to a translated one.

#22 Updated by bertagaz over 2 years ago

intrigeri wrote:

Well, no: we're still concatenating strings, which is never a good idea when l10n matters. So again: the filename should be a gettext placeholder, and not a string appended to a translated one.

Ok, I know close to nothing to gettext, so I'll dig again to get how to fix that. If you have pointers or examples, you're welcome. :)

#23 Updated by intrigeri over 2 years ago

Ok, I know close to nothing to gettext, so I'll dig again to get how to fix that. If you have pointers or examples, you're welcome. :)

I think that all our software (e.g. Tails Installer and Upgrader and probably a number of our scripts in tails.git) have examples of gettext strings that contain a placeholder.

#24 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

I think that all our software (e.g. Tails Installer and Upgrader and probably a number of our scripts in tails.git) have examples of gettext strings that contain a placeholder.

I already did have a look before, but didn't find something. I did again after this note of yours, but given 'gettext placeholder' is a bit vague, it's not so easy to find examples.

If your mentionning the ability to use %s, then I've spent quite some time trying to use this syntax and failed. Note that I didn't find such a thing in our code.

I've found in the tails-spoof-mac script some kind of variable usage in the gettext string, and I've pushed 0d165b5f6dd72fca94908db7c40af2edbc290b1e that uses the same kind of things.

Is it what you had in mind?

#25 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

I already did have a look before, but didn't find something. I did again after this note of yours, but given 'gettext placeholder' is a bit vague, it's not so easy to find examples.

Sorry about that! I wrongly guessed I could stay in reviewing mode, while clearly I should have switched to "teaching shell script internationalization" mode.

Otherwise I would have pointed you to the gettext doc.

If your mentionning the ability to use %s, then I've spent quite some time trying to use this syntax and failed. Note that I didn't find such a thing in our code.

Indeed, I mis-remembered: there's no such thing as %s when doing i18n of shell scripts.

I've found in the tails-spoof-mac script some kind of variable usage in the gettext string, and I've pushed 0d165b5f6dd72fca94908db7c40af2edbc290b1e that uses the same kind of things.

Is it what you had in mind?

Yes, that's it!

But dialog_msg is still built by concatenating localized strings. It should be built from one single localized string.
I didn't check if other strings in this script are affected by similar problems, please do.

#26 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

Sorry about that! I wrongly guessed I could stay in reviewing mode, while clearly I should have switched to "teaching shell script internationalization" mode.

Otherwise I would have pointed you to the gettext doc.

I found it by myself, but was a bit confused by the difference between your words ("placeholder") and the one used in this doc.

But dialog_msg is still built by concatenating localized strings. It should be built from one single localized string.
I didn't check if other strings in this script are affected by similar problems, please do.

Pushed a fix vs string concatenation. There's no other translated strings in this script, so it should be good now.

#27 Updated by sajolida over 2 years ago

Reading the comments after #12375#note-17 I see no mention of the concern I raised there:

Now my main concern is that if the user chooses to keep the current file name, she will be prompt with this message every time which will very quickly become annoying. Could we avoid this? Maybe dropping an empty dotfile to make that file as seen already (.migration.kdbx.keep in my example). Or would this be overkill?

What's your plan regarding this?

#28 Updated by bertagaz over 2 years ago

sajolida wrote:

Reading the comments after #12375#note-17 I see no mention of the concern I raised there:

Now my main concern is that if the user chooses to keep the current file name, she will be prompt with this message every time which will very quickly become annoying. Could we avoid this? Maybe dropping an empty dotfile to make that file as seen already (.migration.kdbx.keep in my example). Or would this be overkill?

What's your plan regarding this?

Right, sorry, forgot to reply. To me that sounds a bit overkill, and taking into account use cases we don't really support. Maybe annoying users will help to get more people using the correct database name? Also, if I get it right, this migration script will at some point be removed (3.1 or 3.2?), so they will be annoyed for not so much time in the end. Let see what intrigeri thinks about that too.

#29 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Info Needed

Pushed a fix vs string concatenation. There's no other translated strings in this script, so it should be good now.

Thanks! What did you test exactly with the latest code? (Just wanna get some confidence before I merge, and save me some time.)

#30 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri

intrigeri wrote:

Pushed a fix vs string concatenation. There's no other translated strings in this script, so it should be good now.

Thanks! What did you test exactly with the latest code? (Just wanna get some confidence before I merge, and save me some time.)

I did not rebuild an ISO from the last commit to test it but did adapt a previously build ISO from this branch by modifying the script the exact same way I've pushed it. I did not test all the features implemented by this script, as we already did test that extensively and nothing else changed. But the zenity window is working as expected.

#31 Updated by intrigeri over 2 years ago

  • QA Check changed from Info Needed to Ready for QA

Thanks!

#32 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 50 to 80
  • QA Check changed from Ready for QA to Dev Needed

Code review passes, except one detail: as most of our wrappers, I think this one should use exec to start the real keepassx binary, so that we don't keep a useless shell process behind us. (I'm aware that at least another wrapper fails to use exec and this wasn't noticed when reviewing it, but that's no reason to introduce the same mistake in another place.)

If you're bored by this long process and prefer me to implement the change I'm asking for, just let me know.

#33 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

Code review passes, except one detail: as most of our wrappers, I think this one should use exec to start the real keepassx binary, so that we don't keep a useless shell process behind us. (I'm aware that at least another wrapper fails to use exec and this wasn't noticed when reviewing it, but that's no reason to introduce the same mistake in another place.)

Good idea!

If you're bored by this long process and prefer me to implement the change I'm asking for, just let me know.

That's fine, I have my good share of responsibility for this long process. I've pushed a commit adding exec around calls to keepassx. Hope that's what you meant.

#34 Updated by intrigeri over 2 years ago

I've pushed a commit adding exec around calls to keepassx. Hope that's what you meant.

Thanks! I'll look into it later today.

#35 Updated by sajolida over 2 years ago

Right, sorry, forgot to reply. To me that sounds a bit overkill, and
taking into account use cases we don't really support.

If there's no simple solution, then I can agree with you.

Maybe annoying
users will help to get more people using the correct database name?

That's right.

Also, if I get it right, this migration script will at some point be
removed (3.1 or 3.2?), so they will be annoyed for not so much time
in the end. Let see what intrigeri thinks about that too.

When was this decided?

I don't think we should assume that people use Tails regularly.
Tails is an alternative and occasional operating system. In that sense I
prefer when we keep migrations working for as long as possible.

At least concerning migration documentation, we agreed on keeping them
as long as they don't require more work from us. If at some point they
don't work anymore or need to be updated, we drop them. Could we do the
same for code?

#36 Updated by intrigeri over 2 years ago

Also, if I get it right, this migration script will at some point be
removed (3.1 or 3.2?), so they will be annoyed for not so much time
in the end. Let see what intrigeri thinks about that too.

When was this decided?

I don't remember any such decision, and would disagree with it for the reasons sajolida puts clearly below.

I don't think we should assume that people use Tails regularly.
Tails is an alternative and occasional operating system. In that sense I
prefer when we keep migrations working for as long as possible.

At least concerning migration documentation, we agreed on keeping them
as long as they don't require more work from us. If at some point they
don't work anymore or need to be updated, we drop them. Could we do the
same for code?

I've probably misunderstood, but I've been working for a while under the assumption that this applied to the code as well.
So: yes, agreed!

#37 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

What sajolida proposed should be a trivial one-liner, so IMO it's worth implementing it.

#38 Updated by intrigeri over 2 years ago

  • Target version changed from Tails_3.0~rc1 to Tails_3.0

I've merged the current state of the branch into feature/stretch as it improves UX compared to what we had previously. Let's please improve it a tiny bit more in 3.0 as proposed by sajolida, hence not closing this ticket.

#39 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

I've probably misunderstood, but I've been working for a while under the assumption that this applied to the code as well.
So: yes, agreed!

Ack, fair enough, didn't remember that. Pushed two commits on top of the related branch to implement that.

#40 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Pushed two commits on top of the related branch to implement that.

This new algorithm seems flawed to me: when the .no_keepassx_db_renaming file exists, it seems to me that we won't be running keepassx at all, which is a regression compared to what we have in 3.0~rc1. Or did I get it wrong?

I think this (untested) is closer to what you meant to write:

--- a/config/chroot_local-includes/usr/local/bin/keepassx
+++ b/config/chroot_local-includes/usr/local/bin/keepassx
@@ -39,14 +39,13 @@ elif mountpoint -q "$PERSISTENT_DATA_DIR" && \
! [ -e "${NEW_DB}" ] && \
[ "$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null | wc -l)" = "1" ]; then
user_db="$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null)" 
-    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ]; then
-        if prompt_for_database_renaming "${user_db}"; then
-            mv "${user_db}" "${NEW_DB}" 
-            exec /usr/bin/keepassx "${NEW_DB}" 
-        else
-            touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" 
-            exec /usr/bin/keepassx "${user_db}" 
-        fi
+    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ] \
+        && prompt_for_database_renaming "${user_db}"; then
+        mv "${user_db}" "${NEW_DB}" 
+        exec /usr/bin/keepassx "${NEW_DB}" 
+    else
+       touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" 
+       exec /usr/bin/keepassx "${user_db}" 
fi

 # There's an old DB but no new one => import the old DB:

However, if we do that, then the "Renaming it to <i>keepassx.kdbx</i> would allow <i>KeePassX</i> to open it automatically in the future" sentence becomes wrong: we're opening any .kdbx file we find anyway, so the only advantage of renaming is to be supported by our next migration paths. So I would actually do that instead:

--- a/config/chroot_local-includes/usr/local/bin/keepassx
+++ b/config/chroot_local-includes/usr/local/bin/keepassx
@@ -39,14 +39,13 @@ elif mountpoint -q "$PERSISTENT_DATA_DIR" && \
! [ -e "${NEW_DB}" ] && \
[ "$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null | wc -l)" = "1" ]; then
user_db="$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null)" 
-    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ]; then
-        if prompt_for_database_renaming "${user_db}"; then
-            mv "${user_db}" "${NEW_DB}" 
-            exec /usr/bin/keepassx "${NEW_DB}" 
-        else
-            touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" 
-            exec /usr/bin/keepassx "${user_db}" 
-        fi
+    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ] \
+        && prompt_for_database_renaming "${user_db}"; then
+        mv "${user_db}" "${NEW_DB}" 
+        exec /usr/bin/keepassx "${NEW_DB}" 
+    else
+       touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" 
+       exec /usr/bin/keepassx
fi

 # There's an old DB but no new one => import the old DB:

This reflects what the dialog prompt says more accurately, avoids running somewhat arbitrary code found with the known-suboptimal find command (due to using shell where it proves not to be a good fit in the end, as usual perhaps), and gives a stronger incentive to rename the DB (and eases future removal of this code).

Please make it clear which ones of the 3 possible cases you've tested (or skipped for some reason) next time you'll reassign to me for QA. Writing down a 2x2 matrix locally may help ensure you're considering them all.

Thanks!

#41 Updated by intrigeri over 2 years ago

We're getting close to the release. I'll take over on Sunday if there's no update earlier, or a clear ETA from bertagaz that works for me. Or I can just formally take over now, so you can focus on your high-prio CI stuff, and I can better organize my own time :)

#42 Updated by bertagaz over 2 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

We're getting close to the release. I'll take over on Sunday if there's no update earlier, or a clear ETA from bertagaz that works for me. Or I can just formally take over now, so you can focus on your high-prio CI stuff, and I can better organize my own time :)

Pushed your commit with your patch which indeed works better in all situations. Bonus point: if the user use a database with a custom name of its own, if she restarts KeepassX in the same session, it will open this custom DB by default, thanks to the KeepassX config file which remind it.

I also pushed a fix removing an exec call that was preventing the folowing shell command to be run.

#43 Updated by intrigeri over 2 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 80 to 100

#44 Updated by intrigeri over 2 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

Woohoo!

#45 Updated by intrigeri over 2 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF