Skip to content

Bugfix 544: Add dialog with extra checkbox for irreversible actions#720

Merged
codinguser merged 6 commits intocodinguser:developfrom
rivaldi8:bug-544-confirm-irreversible-actions
Sep 15, 2017
Merged

Bugfix 544: Add dialog with extra checkbox for irreversible actions#720
codinguser merged 6 commits intocodinguser:developfrom
rivaldi8:bug-544-confirm-irreversible-actions

Conversation

@rivaldi8
Copy link
Copy Markdown
Collaborator

@rivaldi8 rivaldi8 commented Sep 3, 2017

I haven't found any other dialog that should have double confirmation, apart from the ones mentioned on the issue, but I might've missed some.

Fixes #544.

@codinguser
Copy link
Copy Markdown
Owner

codinguser commented Sep 6, 2017

Thanks for the PR.
CI build fails:

org.gnucash.android.test.unit.model.TransactionTest > testCreateAutoBalanceSplit FAILED
    java.lang.NullPointerException at TransactionTest.java:89

@rivaldi8
Copy link
Copy Markdown
Collaborator Author

rivaldi8 commented Sep 6, 2017

Thanks, I didn't notice it had failed (sometimes I don't get notifications when the build fails).

Something is wrong with the tests because they pass in my machine. For some reason, it only fails if I change the locale to en_US.UTF-8.

As expected, the problem was introduced in my split-related changes (2353957). I'll investigate what's going on.

@codinguser
Copy link
Copy Markdown
Owner

ok, that's weird. I just checked it out locally and it builds for me too.
I retriggered the build on Travis and it failed before. I will trigger a third build again and see.
But just go ahead and look at the split stuff again

The user must check a checkbox to enable the delete button. This tries
to avoid confirming irreversible actions by mistake.

Fixes codinguser#544
Showing an AlertDialog directly from a fragment is discouraged. It
doesn't behave properly, for example, when rotating the screen. See
https://developer.android.com/guide/topics/ui/dialogs.html
We'll need other dialogs to have double confirmation.
The user must check a checkbox to enable the delete button. This tries
to avoid confirming irreversible actions by mistake.

Fixes codinguser#544
The user must check a checkbox to enable the delete button. This tries
to avoid confirming irreversible actions by mistake.

Fixes codinguser#544
TransactionTest.testCreateAutoBalanceSplit assumed a default locale with
EUR commodity. The default commodity is set in new Transaction objects,
which might change depending on the environment where the tests are run.
Now we set it explicitly in the tests.
@rivaldi8 rivaldi8 force-pushed the bug-544-confirm-irreversible-actions branch from add808a to 2b0e03e Compare September 13, 2017 16:15
@rivaldi8
Copy link
Copy Markdown
Collaborator Author

I've added a commit that fixes the tests.

You can reproduce the test failure by adding -Duser.language=en -Duser.country=US to org.gradle.jvmargs in gradle.properties. I haven't modified it because it's in .gitignore and I don't know if you did so for some reason. From this discussion it seems it's recommended. It would make running the tests more independent of the environment. In fact, it seems to solve some problems I had with UI tests failing due to locale issues. What do you think?

@codinguser
Copy link
Copy Markdown
Owner

Thanks @rivaldi8
Yeah, I added it to .gitignore on purpose. The properties file contains API keys and release build config info which shouldn't be public.
I assumed that project-level properties file would override my default user-level properties, but this seems not to be the case with gradle - the reverse is true. So I'll consider migrating those variables to my user-level gradle.properties.

@codinguser codinguser merged commit 79aa2fc into codinguser:develop Sep 15, 2017
@rivaldi8 rivaldi8 deleted the bug-544-confirm-irreversible-actions branch October 22, 2017 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants