Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Feature/new error message #864

Merged
merged 4 commits into from
Jul 13, 2020
Merged

Feature/new error message #864

merged 4 commits into from
Jul 13, 2020

Conversation

AlexanderAlferov
Copy link
Contributor

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • If this PR comes from a fork, please Allow edits from maintainers
  • Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
  • Link your Pull Request to an issue - Pull Requests that are not linked to an issue with you as an assignee will be closed, except for minor fixes for typos or grammar mistakes in documentation or code comments.
  • Create Work In Progress [WIP] pull requests only if you need clarification or an explicit review before you can continue your work item.
  • Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)
  • Make sure that your PR does not contain changes in text files, therefore the PR must not contain changes in values/strings/* and / or assets/* (see issue [HELP WANTED][README] Text / Translations  #332 for further information)
  • Make sure that your PR does not contain compiled sources (already set by the default .gitignore) and / or binary files

Description

@AlexanderAlferov AlexanderAlferov added the maintainers Tag pull requests created by maintainers label Jul 13, 2020
@AlexanderAlferov AlexanderAlferov requested review from pwoessner and a team July 13, 2020 11:42
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@pwoessner pwoessner merged commit 3917fac into dev Jul 13, 2020
@pwoessner pwoessner deleted the feature/new-error-message branch July 13, 2020 12:19
@vaubaehn
Copy link
Contributor

vaubaehn commented Jul 16, 2020

Hi,

if I understand the latest code change right, a new single error message is introduced if either a user is affected by Google API Error 10 (STATUS_CODE_GOOGLE_API_FAIL) or bei Google API Error 39508 (STATUS_CODE_REACHED_REQUEST_LIMIT).
IMHO, the consequences of the two different errors (10, 39508) are quite different, and thus users should be informed precisely and seperately.

As far as I understand the problem with API 10, the exposure logging is still active and functional, whilst the risk evaluation/risk detection (and accordingly the infection risk notification?) is broken. This leaves the app quite limited in its functionality for all affected users, as long as the bug is not fixed by google. Just after the bug will have been fixed, users are notified if they encountered a risk of exposure.

In contrast, API 39508 just states, that the quota of callings to the API has been exceeded. In most conditions, this may be just a temporary problem, where the risk evaluation is just delayed until the API is 'open' again. In case of preceeding API 10 this won't be a temporary problem though.

In case a user experiences the API 10 error, the information given by the current error report is not quite correct and misleading.
In Corona-Warn-App/src/main/res/values-de/strings.xml for API 10, line 962 it writes 'Ihre Corona-Warn-App läuft fehlerfrei. Leider können Sie Ihren Risikostatus im Moment nicht aktualisieren. Ihre Risiko-Ermittlung ist weiterhin aktiv und funktioniert. Weitere Informationen finden Sie in unseren FAQ: https://www.coronawarn.app/de/faq/'.
Actually, the correct statement for API 10 affected users should be:
'Ihre Corona-Warn-App läuft zurzeit eingeschränkt. Leider kann Ihr Risikostatus im Moment nicht aktualisiert werden. Die Begegnungs-Aufzeichnung ist aber weiterhin aktiv und funktioniert. Weitere Informationen finden Sie in unseren FAQ: https://www.coronawarn.app/de/faq/'

I therefore suggest to introduce two seperate error messages, one for API 10 and one for API 39508. I know, the disadvantage would be that API 10 effected users then would be confonted with two different error messages, but can it get worse for them either, compared to the current state?

In sum, my suggested changes would be:

In cwa-app-android/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/exception/reporting/ExceptionReporter.kt
line 41+

    if (this.statusCode == STATUS_CODE_GOOGLE_UPDATE_NEEDED) {
        errorMessage = R.string.errors_google_update_needed
    } else if (this.statusCode == STATUS_CODE_GOOGLE_API_FAIL) {
       errorMessage = R.string.errors_google_api_error
    } else if (this.statusCode == STATUS_CODE_REACHED_REQUEST_LIMIT) {
        errorMessage = R.string.errors_google_reached_request_limit
    }

In cwa-app-android/Corona-Warn-App/src/main/res/values-de/strings.xml
line 1004+

    <!-- XTXT: error dialog - Google API Error (10)  -->
    <string name="errors_google_api_error">"Fehler bei Kommunikation mit Google API (10): Ihre Corona-Warn-App läuft zurzeit eingeschränkt. Leider kann Ihr Risikostatus im Moment nicht aktualisiert werden. Die Begegnungs-Aufzeichnung ist aber weiterhin aktiv und funktioniert. Das Problem wird zu einem späteren Zeitpunkt von Google behoben. Ihre aufgezeichneten Begegnungen bleiben erhalten, solange Sie die App nicht löschen oder neu installieren. Weitere Informationen finden Sie in unseren FAQ: https://www.coronawarn.app/de/faq/#API10 \n"</string>
    <!-- XTXT: error dialog - Google API Error (39508) reached request limit per day -->
    <string name="errors_google_reached_request_limit">"Vorübergehender Fehler bei Kommunikation mit Google API (39508): Leider können Sie Ihren Risikostatus im Moment nicht aktualisieren. Bitte versuchen Sie es später noch einmal. Sind Sie nur von diesem Fehler betroffen, läuft Ihre Corona-Warn-App trotzdem fehlerfrei, und die Begegnungs-Aufzeichnung ist weiterhin aktiv. Ihre aufgezeichneten Begegnungen bleiben erhalten, solange Sie die App nicht löschen oder neu installieren. Weitere Informationen finden Sie in unseren FAQ: https://www.coronawarn.app/de/faq/#API39508 \n"</string>

Suggested English translations for Corona-Warn-App/src/main/res/values-en/strings.xml:
line 1003+

    <!-- XTXT: error dialog - Google API Error (10)  -->
    <string name="errors_google_api_error">"Error when communicating with Google API (10): Your Corona-Warn-App is currently running with limited functionality. Unfortunately, your risk status cannot be updated at the moment. Exposure logging remains active and is working correctly. Google will fix the problem soon. Your logged exposures will remain intact as long as you do not delete or reinstall the app. For further information, please see our FAQ page: https://www.coronawarn.app/en/faq/#API10 \n"</string>
    <!-- XTXT: error dialog - Google API Error (39508) reached request limit per day -->
    <string name="errors_google_reached_request_limit">"Temporary Error when communicating with Google API (39508): Unfortunately, we cannot update your current risk status at the moment. Please try again later. If you are affected by this error only, the Corona-Warn-App is otherwise running correctly, and exposure logging remains active. Your logged exposures will remain intact as long as you do not delete or reinstall the app. For further information, please see our FAQ page: https://www.coronawarn.app/en/faq/#API39508 \n"</string>

by the way: the corresponding strings for Corona-Warn-App/src/main/res/values/strings.xml are still missing?

I hope this suggestion is useful and maybe commits to the project.
Pardon the form of this commit, I'm on github for the very first time.

Cheers, vaubaehn

Off-topic: Let's hope google will fix the the API problems soon... if you take android download rates of CWA and the following resources into account,
https://www.heise.de/news/Corona-Warn-App-iPhone-Nutzer-angeblich-ueberrepraesentiert-4791616.html
https://www.connect.de/ratgeber/android-versionen-verteilung-ueberblick-liste-3200452-8635.html
you may estimate the number of affected Android 6 users with API 10 and a not working risk evaluation at large and by to 1.000.000 users in Germany...

edit: inserted 'code tags' to the strings-section

edit 2: added additional text according to the suggestion from @tkowark made here: #816 (comment)

edit 3: small enhancement of English translation

edit 4: In German strings, replaced "Protokollieren Ihrer Begegnungen" and similar with the official and already used term "Begegnungs-Aufzeichnung"

@vaubaehn
Copy link
Contributor

Hi @tkowark , as you mentionend here (#816 (comment)), you consider to take above proposed changes to the product management. If you are somehow flexible in planning your meetings, I kindly ask you to wait a little. Meanwhile, in the week-end, I had an idea on how to much better handle already known exceptions, issues etc. regarding the UX.

The above changes from last week would create more user-friendly error messages within dialog-boxes. I guess, this may be implemented very fast, however it is not the best solution in my opinion.

My new idea would introduce a new card "App- & Systemstatus" (working title) inside the home screen of CWA, placed below the "Risk Status" card, presenting a quick and very brief overview to the correct functioning of CWA, ENS and basic device functions (in ios, this card also may replace the "app information"). If there were any caught exceptions or other issues, that may be monitored from within CWA, a brief information (without any details) may be shown here, otherwise just the information "everything is working correctly". Clicking on that card then opens a new screen/view, that presents few app information-cards and issue-cards, one for each information/issue, again with a very brief information about each issue (for example, one card for API 10, if that exception had been caught before). Clicking on one card then opens a new screen, presenting more in-depth information about that specific issue (for example, what is API 10 error, how does it impact on the functionality of CWA, how it might be resolved, links to FAQ, etc.).

I would try to present a more detailed and clear sketch for "App- & Systemstatus" until tomorrow evening in https://github.com/corona-warn-app/cwa-wishlist.
If you, after your kind review, consider handling errors and issues via "App- & Systemstatus" to be a good idea, then you might want to introduce that new concept to the product management, also.
If product management agrees, that "App- & Systemstatus" way of error handling enhances the UX and should be implemented, one could think of a two-step-process of implementation:
First, in the very next release of CWA, exception handling via dialog boxes (like proposed last week), might be implemented.
Second, in a future release of CWA, app-information/exception/issue handling via cards may be implemented, as that would be much more of effort to do so.

Kind regards, V.

@tkowark
Copy link
Member

tkowark commented Jul 20, 2020

Hi @vaubaehn ,

thanks again for the new proposal. Given the extent of it, going the wishlist route is definitely the right way to go. In the meantime, we will still need a better short term solution for existing issues and hence will continue to discuss them with the PM. And no worries, this is not a once in a month kind of thing but we can discuss proposals more frequently.

Looking forward to the full proposal!

@vaubaehn
Copy link
Contributor

vaubaehn commented Jul 30, 2020

@tkowark @jakobmoellersap @pwoessner @AlexanderAlferov

Hi, I guess you're about to release 1.2.0, so I want to quickly draw attention to here.
17 days ago changes have merged into the current dev 1.2.0 on how to handle error messages for API 10 and API 39508. In the current release stage, a combined 'business error message' for both errors is presented to the user. In branch 1.1.1 a generic error message was shown.

The problem with the current state is, that

  • message of API 10 and API 39508 are now combined, even they may occur independently,
  • in the business error message there is no root cause presented anymore (API 10 or API 39508)
  • thus the link to help via FAQ on website is broken
  • thus the support team for Google Play store may run into problems helping, because they can't differ anymore what is the cause of the problem reported by user in the store

Still a certain percentage of users may run into API 10 or API 39508,

  • when their update to ENF 1.5 is delayed for some reason,
  • when they reactivate an old phone, where it may take up to 2 weeks or longer until the necessary updates of GPS, GMS, ENF are pushed to the phone.

Also, combining these messages and just telling everything works fine may be interpreted as 'obfuscating' problems as discussed in media and pubilc in recent days, which might be not in your intention.

I guess, this issue here was just overseen, so this is a kind pointer 👍

All the best for the next release! Kind regards
V.

Edit: In case your want to change current handling:

@vaubaehn
Copy link
Contributor

vaubaehn commented Aug 6, 2020

Hi @tkowark @jakobmoellersap @pwoessner @AlexanderAlferov,
according to the new version 1.2.0 that was just released, error messages for API 10 and API 39508 are now combined in a more unspecific business error message (line 1046):
<string name="errors_google_api_error">"Ihre Corona-Warn-App läuft fehlerfrei. Leider können Sie Ihren Risikostatus im Moment nicht aktualisieren. Ihre Risiko-Ermittlung ist weiterhin aktiv und funktioniert. Weitere Informationen finden Sie in unseren FAQ: https://www.coronawarn.app/de/faq/"</string>

Will the error number (either API 10 or API 39508) be shown to the user in the error dialog (maybe in the dialog's title)?
If yes, everything is fine.
If not, then the website/FAQ needs to be changed, because the users with theses exceptions don't know where to look for inside the FAQ, the support-team for google play store and also the technical helpline should be informed, that users with an unspecific message may show up...

@vaubaehn
Copy link
Contributor

@kbobrowski , referenced from #1021 (comment)

I think CWA should maintain internal count of calls and simply not attempt to call if the quota is already exceeded. What would be very useful is to instead display the information about it (perhaps in another UI section), together with information about original error, which is currently not displayed to the user if it has happened in the background.

I was already having something like this in mind some time ago (#864 (comment)).
The concept I have in mind would create a new viewmodel in MainFragment.kt (reliabilityViewModel) that provides information about device status in terms of reliability of CWA's functions and results, depending on device- and OS-specifications, as well as encountered errors at a glance (as a seperate card) with short text and colored symbols, for example:

  • Fully functional (green checked symbol)
  • functional with temporary/minor problems (yellow? blue? i(nformation)-symbol)
  • not functional (red cross symbol)

A click on that card opens a new fragment, that shows seperate cards for each evaluated function:

  • update regularly (green: once per day; yellow: one day missed; red: two or more days missed)
  • ENF is implemented (connected; available) (green: yes; red: no) => caught exception API 17 = no
  • ENF is uptodate (green: yes; yellow: no) => needs to be checked by calling latest API functions and catching exception if not available (as suggested by google => no exception = ENF is uptodate)
  • Bluetooth is enabled (green: yes; yellow: no)
  • BLE beacons can be sent (green: yes; yellow: no)
  • BLE beacons have been received (green: yes; yellow: no; white: pending for data privacy) => needs new implemented feature in ENF (either own method, or ProvideDiagnosisKeys() returns -1 when no beacons are present.
  • problems with update process (green: no; yellow: yes) -> API 39508
  • problems with 2001, 4000, 9002 time out, 9002 timed out waiting for 60000 ms, 9002 http web request 901...

To reduce confusion, only basic and important, and non-redundant information should be displayed, For example, no need for a 'bluetooth is enabled card', when it's turned on.

A click on a card in that 2nd fragment opens new fragment with detailed information and troubleshooting for each issue:

  • 2001: activate T-Systems certificate (how to)
  • 39501: wait 24 hours
  • BLE-beacons cannot be sent: CWA may be used for receiving risk encounters, but cannot be used to transmit positive test result/warn others
  • ENF is not implemented: give information
    ...

This would

  • help to handle exceptions/errors, giving information at the right place
  • thus mitigate work load for support teams
  • help users who complain that they don't have any idea, if the app even works (as seen often in reviews in Google Play Store)

Do you feel like working with me on such a concept (no coding) to put it to the CWA-wishlist, or to discuss some points? We could meet for that in your repo or my (empty) repo...
As the concept is very modular, I'm for now just talking about the very basics / frame, not every detail that might be implemented

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants