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

Risk Calculation Errors - Alert Refinement #868

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

SebastianWild
Copy link
Contributor

Checklist

  • This PR does not change text that hasn't been signed off with the RKI. Have a look at the pinned issue 440
  • 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.
  • 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)

Description

Previously, UIAlertControllers triggered by the app because of a failed exposure detection (when an ExposureDetection.DidEndPrematurelyReason is thrown) would have a simple OK button and nothing else. This PR adds some more details, as well as links several ENErrors to their corresponding FAQ entries:

  • ENError(.unsupported) -> FAQ
  • ENError(.internal) -> FAQ
  • ENError(.rateLimited) -> FAQ

Alert before:
IMG_55CC76CD50C1-1

Alert after:
IMG_F7288C2C29BF-1

- Updated Strings
- Now directs to FAQ for ENErrors .unsupported, .internal, and .rateLimited
@SebastianWild SebastianWild requested review from kaluju and a team July 8, 2020 01:03
@SebastianWild SebastianWild self-assigned this Jul 8, 2020
Comment on lines +22 to +31
/* General - Links */

"General_moreInfo_URL" = "https://www.coronawarn.app/de/faq/";

"General_moreInfo_URL_EN5" = "https://www.coronawarn.app/de/faq/#ENError5";

"General_moreInfo_URL_EN11" = "https://www.coronawarn.app/de/faq/#ENError11";

"General_moreInfo_URL_EN13" = "https://www.coronawarn.app/de/faq/#ENError13";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shifted these around as they are used in several places (not just ExposureSubmission) anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote the Exposure Submission error handlings to use these links, removed the old traces of the links.

default: return nil
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extension seems like it could go into a separate file, but I wasn't sure if it's worth it as it's only used here.

It is not private as we'd like to unit test it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to a separate extension, and added the ExposureSubmission stuff into it as well. Check Error+FAQUrl.swift for this change.

static let moreInfoURL = AppStrings.Links.appFaq
static let moreInfoURLEN5 = AppStrings.Links.appFaqENError5
static let moreInfoURLEN11 = AppStrings.Links.appFaqENError11
static let moreInfoURLEN13 = AppStrings.Links.appFaqENError13
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left these be in order to not change too many unrelated files in this PR, but we can certainly adjust the references where they're used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to adjust these in the Exposure Submission flow to get rid of the indirection.

@inf2381 inf2381 added the enhancement Improvement of an existing feature label Jul 8, 2020
@haosap
Copy link
Member

haosap commented Jul 8, 2020

Hi @kaluju ,
Could you please review the resources files?
Thanks.

Copy link
Contributor

@kaluju kaluju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new error message looks fine.

@melloskitten
Copy link
Member

PLEASE DO NOT MERGE THIS. I want to do adjustments.

@melloskitten
Copy link
Member

I have merged the exposure submissions errors & the errors introduced in this pull request to the best of my knowledge, in order to remove duplication and have a more pleasant code base. Thanks to @johannesrohwer for helping me check for consistencies. Please check the changes again, if you'd be so kind.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants