Switch error reporting over to ACRA @ ankidroid.org

[timrae]

This PR switches the AnkiDroid error reporting over to ACRA / Acralyzer on ankidroid.org and makes the error reporting occur automatically in the background by default (a toast is shown so the user knows). This is not quite ready to merge as I need to finalize some details about the couchdb authentication with @agrueneberg

See the discussion on the forum for more background info

[agrueneberg]

From a privacy standpoint, I think REPORT_ASK should be the default, not REPORT_ALWAYS.

[agrueneberg]

I think it makes more sense from a UX standpoint as well: toasts are easily missed in the heat of the moment, and the dialog could say something like "We're sorry / AnkiDroid has crashed. Please tell us about the crash so that we can fix it." Is it possible to rename the "OK" button to "Report"?

[timrae]

I agree on the privacy point, but not so much on the UX standpoint.

We have several places in the code where we report on caught exceptions -- i.e. we prevent a crash through exception handling, but still want to send info for monitoring and debugging purposes. In these cases, having a dialog pop-up everytime is going to be disruptive and a fairly negative user experience. More generally I think making users read and press OK on a dialog every time increases the frustration with a crash.

How about we show a consent dialog along with the privacy policy the first time a crash is reported, and strongly suggest the user to enable auto-reporting?

[timrae]

Is it possible to rename the "OK" button to "Report"?

Not in ACRA 4.5.0, but it will be in 4.6 which will hopefully be coming out soon

[timrae]

Also, currently the privacy policy says that we don't collect the user's email address, but now there is a possibility we collect this via the logcat, since the sync logs include the ankiweb username. Should we update the privacy policy to reflect this?

Thoughts @nicolas-raoul?

[nicolas-raoul]

Good catch!

Could we maybe avoid logging the ankiweb username?
Or put this log at DEBUG level, which means it would not be generated for release APKs.
Or maybe insert asterisks, or replace the email with its secure checksum.

The ACRA server is probably not extremely secure, and leaking user emails would be sad. Not collecting emails in the first place would protect us.

[timrae]

I just checked, and actually the sync logs are already set to the debug level:
https://github.com/ankidroid/Anki-Android/blob/develop/AnkiDroid/src/main/java/com/ichi2/libanki/Collection.java#L1559

So probably we don't have to worry about that.

Currently the "disclaimer" says:

Disclaimer: We don't collect any personal information such as your email address, phone number, IP address or your phone IMEI. We do collect some information about your device such as the manufacturer, model and version of Android, as well as the nature of reported errors themselves.

IP address is currently getting logged actually... I looked into it, and while it's possible to prevent it from being explicitly included in the data transmitted in the crash report, at the end of the day the ip address is still available to us, and acralyzer takes note of it. I don't think anyone could really consider IP address to be private information anyway; can we just remove this part from the disclaimer?

[nicolas-raoul]

Yes, I guess IP address can be removed from the disclaimer.
The disclaimer probably dates from the time when these reports were sent by email, that's why IP was not sent.

[timrae]

Yes, I guess IP address can be removed from the disclaimer.

Done

From a privacy standpoint, I think REPORT_ASK should be the default

Done. However, I plan to submit a PR to ACRA to allow adding an "Automatically send error reports" checkbox to the error reporting dialog, which will be checked by default the first time the dialog is shown.

Is it possible to rename the "OK" button to "Report"?

Done

[timrae]

I've sent over a PR to Acra which allows using a custom dialog. This will allow us to easily add an "Always report errors" checkbox, which I plan to have checked by default the first time the dialog is shown to the user.

For now I'm going to merge this and release some new beta versions

[flerda]

I noticed that Travis says that this pull request broke one of the tests:
https://travis-ci.org/ankidroid/Anki-Android/builds/47480129

com.ichi2.anki.tests.MediaTest > testAdd[test(AVD) - 4.4.2] FAILED 
    java.lang.NullPointerException
    at com.ichi2.libanki.Collection._renderQA(Collection.java:993)
:AnkiDroid:connectedAndroidTest FAILED

Would you mind having a look?

[timrae]

Pretty sure that was because of an infinite loop in the upgrade path.
Bumping the version number to beta3 should have resolved it.
On 19/01/2015 5:55 pm, "Flavio Lerda" notifications@github.com wrote:

I noticed that Travis says that this pull request broke one of the tests:
https://travis-ci.org/ankidroid/Anki-Android/builds/47480129

com.ichi2.anki.tests.MediaTest > testAdd[test(AVD) - 4.4.2] FAILED
java.lang.NullPointerException
at com.ichi2.libanki.Collection._renderQA(Collection.java:993)
:AnkiDroid:connectedAndroidTest FAILED

Would you mind having a look?

—
Reply to this email directly or view it on GitHub
#681 (comment)
.

[timrae]

I took a look at the build history and that doesn't seem to have been the issue since it's still failing on subsequent commits after bumping the version:

https://travis-ci.org/ankidroid/Anki-Android/builds/47482041

[timrae]

It seems that ACRA may be interfering with the test somehow, though I've checked that Media is actually added properly when the app is run for real. I've asked on the ACRA page:
ACRA/acra#224

[hssm]

This is bizarre. Running the tests locally in debug mode works normally and everything passes. Running them in run mode gives me the same error that shows up on Travis. Seems to be failing to create an AnkiDroidApp instance so sInstance is null inside it.

[hssm]

Although commenting out the Acra.init line like you mentioned makes it work again. It's kind of harder to debug when the error doesn't show up in debug mode.

[timrae]

Right?! Maybe it's a threading issue...?
On 20/01/2015 8:38 am, "Houssam Salem" notifications@github.com wrote:

Although commenting out the Acra.init line like you mentioned makes it
work again. It's kind of harder to debug when the error doesn't show up in
debug mode.

—
Reply to this email directly or view it on GitHub
#681 (comment)
.

[hssm]

We could be sneaky and move the sInstance = this; line to the top and pretend this never happened. That's the easy solution.

[timrae]

We can try that, though it seemed it was still sometimes failing for me
when I tested that yesterday. It was behaving very strange.

Not sure how useful it will be but the acra guys just asked for a full
stack trace if you have one handy?
On 20/01/2015 8:41 am, "Houssam Salem" notifications@github.com wrote:

We could be sneaky and move the sInstance = this; line to the top and
pretend this never happened. That's the easy solution.

—
Reply to this email directly or view it on GitHub
#681 (comment)
.

[hssm]

It really is a race condition. I can reproduce the issue by replacing Acra.init with Thread.sleep(200), so Acra isn't at fault, only the extra time it takes to initialize it.

[timrae]

Interesting... I've also found that if you add Thread.sleep(10000) to the
end of onCreate(), the test usually passes (not always), but the test
completes in less than 10s... so the test mechanism is definitely not
waiting for onCreate() to complete, which presumably means that it's being
run in a separate thread

[hssm]

Yes, that's exactly what is happening. The main application and the test runner execute on separate threads. I have no idea how it makes sense for the test runner to start testing before the application has finished initializing, however. Either we are doing something wrong or this particular test framework isn't very advanced and we need some additional code to make it wait (but then we have to duplicate this code for each new test class we make).

[timrae]

This thread has a similar question, though not exactly the same since they
are saying the bug has been fixed, whereas it obviously hasn't been for us
http://stackoverflow.com/questions/6516441/why-does-androidtestcase-getcontext-getapplicationcontext-return-null

On Tue, Jan 20, 2015 at 11:03 AM, Houssam Salem notifications@github.com
wrote:

Yes, that's exactly what is happening. The main application and the test
runner execute on separate threads. I have no idea how it makes sense for
the test runner to start testing before the application has finished
initializing, however. Either we are doing something wrong or this
particular test framework isn't very advanced and we need some additional
code to make it wait (but then we have to duplicate this code for each new
test class we make).

—
Reply to this email directly or view it on GitHub
#681 (comment)
.

[timrae]

@nicolas-raoul
I though debug logs were automatically stripped at runtime, but that doesn't seem to be the case actually... And the current script is also commenting out info, which is not really helpful for analyzing crash reports. I've added issue 2483