Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences #3003

30mar · 2021-06-24T14:17:14Z

Description

Add Flow and Logic to Adding new local statistics
Add filtering mechanism
The remaining TO DOs are just a guidance on where to integrate the network calls and the persistence

Obstacles to work on:
1- moving the content Offset of the scrollView without animation.
2- the back button on the district navigation toolbar

Link to Jira

https://jira-ibs.wbs.net.sap/browse/EXPOSUREAPP-7687

Screenshots

RPReplay_Final1624543706.MP4

NataliaLemmerth

UA approved

…branch' into feature/7087-Statistic-data-local-7-day-incidence/7687-UI-7-day-local-incedences

naveeddotio · 2021-06-24T15:55:08Z

.../ENA/Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddLocalStatisticsModel.swift

+		})
+	}
+
+//	mutating func save() {


For future: could be renamed to what is being save, save is a bit generic.

...NA/ENA/Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddStatisticsCardView.swift

naveeddotio · 2021-06-24T16:00:56Z

src/xcode/ENA/ENA/Source/View Helpers/AppStrings.swift

@@ -1367,7 +1367,11 @@ enum AppStrings {

 	enum Statistics {
 		static let error = NSLocalizedString("Statistics_LoadingError", comment: "")
-
+		enum AddCard {
+			static let SevenDayIncidence = NSLocalizedString("Statistics_Add_SevenDayIncidence", comment: "")


sevenDayIncidence

naveeddotio · 2021-06-24T16:06:20Z

...NA/ENA/Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddStatisticsCardView.swift

+
+enum CreatedLocalStatisticsState {
+	case empty // 0
+	case NotYetFull // 0 < number of local statistics cards < 5


I would ditch the whole state which can lead to complications if not updated correctly and create a computed var:

var canAddMore: Boo { return currentCount < threshold }

States are bad!

@whiskey we have 3 have three states o:
1- show only add button if there number of cards = 0
2- show both add and edit buttons when 0 < number < 5
3- disable add button but leave the edit button enabled when 5 < number

I know the use case but don't think it's the best solution here because if I miss out to update the state after removing a cell, it might get corrupted. Let's leave it for now but I'd like to test out my 'computed approach' as well and see if this is easy to implement w/o any dependencies to the store.

naveeddotio · 2021-06-24T16:13:09Z

.../Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddStatisticsCardsViewModel.swift

+
+	init(
+		localStatisticsModel: AddLocalStatisticsModel,
+		presentSelectStateList: @escaping (SelectValueViewModel) -> Void,


shouldn't it be presentFederalStatesList?

What is the difference between a State and federalState ?

A state can be a state in the app, so not to confuse the new developer, also it's "plural" States as is a list of FederalStates

naveeddotio · 2021-06-24T16:13:44Z

.../Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddStatisticsCardsViewModel.swift

+	init(
+		localStatisticsModel: AddLocalStatisticsModel,
+		presentSelectStateList: @escaping (SelectValueViewModel) -> Void,
+		presentSelectDistrictList: @escaping (SelectValueViewModel) -> Void


shouldn't it be presentSelectDistrictsList?

whiskey

Some minor questions and tweaks. Happy to discuss my points.

whiskey · 2021-06-24T17:54:21Z

.../ENA/Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddLocalStatisticsModel.swift

+			let jsonData = try Data(contentsOf: jsonFileURL)
+			self.allDistricts = try JSONDecoder().decode([DistrictElement].self, from: jsonData)
+		} catch {
+			Log.debug("Failed to read / parse district json", log: .ppac)


Why no error then? It's more serious than just a debug log

good point, need to discuss this with max

No I just mean logging the error as Log.error instead of .debug. Decoder errors are definitely worth noting!

Suggested change

Log.debug("Failed to read / parse district json", log: .ppac)

Log.error("Failed to read / parse district json: \(error)", log: .ppac, error: error)

error should not contain any critical information and can be dumped in the logs

...NA/ENA/Source/Scenes/Home/Cells/Statistics/AddStatisticsCardView/AddStatisticsCardView.swift

src/xcode/ENA/ENA/Source/Scenes/SelectValueViewController/SelectValueTableViewController.swift

whiskey · 2021-06-24T18:08:46Z

src/xcode/ENA/ENA/Source/View Helpers/AppStrings.swift

-
+		enum AddCard {
+			static let SevenDayIncidence = NSLocalizedString("Statistics_Add_SevenDayIncidence", comment: "")
+			static let fromTheWholeCountry = NSLocalizedString("Statistics_Add_fromTheWholeCountry", comment: "")


Just wondering, what is the difference between "whole country" and "nationwide"? 🤔

They are actually different texts in German in the Design
the first is translated in german as "Gesamtes Bundesland" , the other is "Bundesweit"

Aaahh. I would think suggest something like 'whole state' then because 'country' is more the whole of Germany than just a… state/Bundesland.

…ctValueTableViewController.swift Co-authored-by: Carsten Knoblich <carsten@staticline.de>

…sticsCardView/AddStatisticsCardView.swift Co-authored-by: Carsten Knoblich <carsten@staticline.de>

…UI-7-day-local-incedences' of github.com:corona-warn-app/cwa-app-ios into feature/7087-Statistic-data-local-7-day-incidence/7687-UI-7-day-local-incedences

30mar added 21 commits June 24, 2021 15:22

Add New Files for local statistics

a1addd4

Add Add button text

a836eab

update image enum for ppa

1ad9238

handle presentation from coordination

274d2e6

pass completions from Statistics Cell to the HomeCoordinator

5d8a87b

Add configuration for AddLocalStatistics view

2d1dbb7

Add LocalStatisticsModel With helper functions

d22b6e8

Add viewModel

89b71f2

Add AddLocalStatistics UI

a5b7d69

Add Custom View to handle dashed outline

e7220f6

Add Enum for State ID handling

9c82c5f

re-arrange arguments

74830b5

disable first cell for State and District selection

12d4df8

extend image type for selectionCell

8963391

update ppa unit tests

ee2aa89

handle right image in selection cell

c03370e

disable first cell for local statistics selection

6423c0f

update unit tests

3acb135

add logging for local statistics

9999e2d

add Appstings

02e28c6

handle dismiss to root

250ea9e

30mar requested review from a team June 24, 2021 14:17

NataliaLemmerth approved these changes Jun 24, 2021

View reviewed changes

30mar added 2 commits June 24, 2021 16:47

fix initial position of the Statistics cards

8fdc528

update tests

7f2e990

30mar added the task part of a feature label Jun 24, 2021

30mar added this to the v2.6.0 milestone Jun 24, 2021

30mar added 2 commits June 24, 2021 17:03

Merge branch 'feature/7087-Statistic-data-local-7-day-incidence/story…

70860c2

…branch' into feature/7087-Statistic-data-local-7-day-incidence/7687-UI-7-day-local-incedences

fix the scrollView Offset again

bc809cc

naveeddotio suggested changes Jun 24, 2021

View reviewed changes

naveeddotio reviewed Jun 24, 2021

View reviewed changes

whiskey suggested changes Jun 24, 2021

View reviewed changes

naveeddotio changed the title ~~Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incedences~~ Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences Jun 25, 2021

30mar and others added 5 commits June 25, 2021 09:33

Update src/xcode/ENA/ENA/Source/Scenes/SelectValueViewController/Sele…

11763e7

…ctValueTableViewController.swift Co-authored-by: Carsten Knoblich <carsten@staticline.de>

Update src/xcode/ENA/ENA/Source/Scenes/Home/Cells/Statistics/AddStati…

aed2b59

…sticsCardView/AddStatisticsCardView.swift Co-authored-by: Carsten Knoblich <carsten@staticline.de>

fix typos

0376bf7

Merge branch 'feature/7087-Statistic-data-local-7-day-incidence/7687-…

04a6dce

…UI-7-day-local-incedences' of github.com:corona-warn-app/cwa-app-ios into feature/7087-Statistic-data-local-7-day-incidence/7687-UI-7-day-local-incedences

disable swift lint rule for arguments count

4da4e61

whiskey approved these changes Jun 25, 2021

View reviewed changes

naveeddotio self-requested a review June 25, 2021 09:24

naveeddotio approved these changes Jun 25, 2021

View reviewed changes

refactoring

46bd977

freshking approved these changes Jun 25, 2021

View reviewed changes

30mar merged commit 26be896 into feature/7087-Statistic-data-local-7-day-incidence/storybranch Jun 25, 2021

30mar deleted the feature/7087-Statistic-data-local-7-day-incidence/7687-UI-7-day-local-incedences branch June 25, 2021 09:34

dsarkar mentioned this pull request Jul 13, 2021

[Solved]: [Display “Local 7-Days-Inzidenz”]: Additional Information in CWA, based on v1.11.0,… corona-warn-app/cwa-wishlist#353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences #3003

Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences #3003

30mar commented Jun 24, 2021

NataliaLemmerth left a comment

naveeddotio Jun 24, 2021 •

edited

Loading

naveeddotio Jun 24, 2021

naveeddotio Jun 24, 2021

whiskey Jun 24, 2021

30mar Jun 25, 2021

whiskey Jun 25, 2021

naveeddotio Jun 24, 2021

30mar Jun 25, 2021

naveeddotio Jun 25, 2021

naveeddotio Jun 24, 2021 •

edited

Loading

whiskey left a comment

whiskey Jun 24, 2021

30mar Jun 25, 2021

whiskey Jun 25, 2021

whiskey Jun 25, 2021

whiskey Jun 24, 2021

30mar Jun 25, 2021

whiskey Jun 25, 2021

	Log.debug("Failed to read / parse district json", log: .ppac)
	Log.error("Failed to read / parse district json: \(error)", log: .ppac, error: error)

Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences #3003

Feature/7087 statistic data local 7 day incidence/7687 UI 7 day local incidences #3003

Conversation

30mar commented Jun 24, 2021

Description

Link to Jira

Screenshots

NataliaLemmerth left a comment

Choose a reason for hiding this comment

naveeddotio Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naveeddotio Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

whiskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naveeddotio Jun 24, 2021 •

edited

Loading

naveeddotio Jun 24, 2021 •

edited

Loading