Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Landing Screen, Notifications Screen, and updated all Onboarding Pages #4

Merged
merged 31 commits into from
Feb 16, 2023

Conversation

ckunchur
Copy link
Contributor

@ckunchur ckunchur commented Feb 9, 2023

Name of the PR

♻️ Current situation & Problem

Describe the current situation (if possible, with an exemplary (or real) code snippet and/or where this is used)

I successfully implemented a conditionally rendered landing screen and updated all of the onboarding page content to walk through the app features, consent, and health data sharing permissions. I also implemented a notifications screen to give a preview of what upload status notifications will look like.

ISSUE:
After creating a fake ECG data point in the Simulator's Health App, I am unable to see the FHIR encoded JSON in the "Mock Upload" tab in the PAWS app, even after rebuilding, deleting/relaunching the app, etc.

The potential sources of error include the current method of binding for checking if onboarding was completed and a change was made 2 commits ago that permanently sets the completedOnboarding variable to be false. However, it is unclear why the onboarding process would be related to the mock upload tab's ability to display data.

Please link any open issue that is addressed with this PR
Shriya's PR has successful viewing of the JSON file in the mock upload tab and should be used as a point of comparison for detecting differences.

💡 Proposed solution

Describe the solution and how this affects the project and internal structure

⚙️ Release Notes

Add a summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented if it appends or changes the public interface.

➕ Additional Information

Provide some additional information if possible

Related PRs

Reference the related PRs
Shriya's PR has successful viewing of the JSON file in the mock upload tab and should be used as a point of comparison for detecting changes.

Testing

Are there tests included? If yes, which situations are tested, and which corner cases are missing?

Reviewer Nudging

Where should the reviewer start? Where is a good entry point?

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Hi @ckunchur,

A great job with this PR!
Amazing to see what you and the team have achieved so far.

I have added a lot of comments in the code to improve it and facilitate its readability in the future. Sorry for the number of comments; I tried to make them very easy fixes by providing review suggestions in almost all of them to incorporate them more easily.

I have identified a few problems with you HealthKit Module when debugging the code and I have created a PR in the main CardinalKit repo to address this: StanfordSpezi/Spezi#46 ... once this is done you should be able to update to the new version and should get all the bug fixes including a fix that will reenable the collection of HealthKit data for you.

You can easily resolve a large number of SwiftLint comments using the swiftlint --fix command from your command line in the root of your project (where the .swiftlint.yml file is located).

Your next steps would be:

  • Address the different comments
  • Resolve the SwiftLint warnings (most of them can be resolved automatically using swiftlint --fix)
  • Address the REUSE specification errors and adjust the UI tests to reflect the new onboarding flow
  • Merge the PR, I have approved the PR, and you can merge them after all the issues above are resolved 🥳🚀

I am happy to help with any of these steps; let me know if and how I can assist you there!

PAWS/PAWS.swift Outdated Show resolved Hide resolved
PAWS/PAWS.swift Outdated Show resolved Hide resolved
PAWS/PAWS.swift Outdated Show resolved Hide resolved
PAWS/Home.swift Outdated Show resolved Hide resolved
PAWS/Home.swift Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #4 (b862063) into main (2d8d895) will decrease coverage by 1.24%.
The diff coverage is 90.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
- Coverage   91.40%   90.16%   -1.24%     
==========================================
  Files          10        9       -1     
  Lines         372      264     -108     
==========================================
- Hits          340      238     -102     
+ Misses         32       26       -6     
Impacted Files Coverage Δ
PAWSUITests/OnboardingTests.swift 73.57% <72.23%> (-6.29%) ⬇️
PAWS/Home.swift 86.96% <100.00%> (ø)
PAWS/PAWS.swift 100.00% <100.00%> (ø)
PAWS/PAWSAppDelegate.swift 100.00% <100.00%> (ø)
PAWSUITests/ContactsTests.swift 100.00% <100.00%> (ø)
PAWSUITests/HealthKitUploadTests.swift 100.00% <100.00%> (ø)
PAWSUITests/NotificationsTests.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8d895...b862063. Read the comment docs.

@PSchmiedmayer
Copy link
Member

PSchmiedmayer commented Feb 15, 2023

@ckunchur I took a look at the failing tests and modified them a bit to reflect the new structure in the application. You can see the diff here: 5091cdb

Regarding the failing UI test for the Mock Upload: The UI test was entering step count information into the Health App using the try exitAppAndOpenHealth(.steps) call. As you have changed the app delegate to no longer collect steps the application was no longer listening to changes in step count in the Health App (HealthKit) and therefore did not trigger an upload in the Mock Upload part of the app.
I changed the UI test to enter an ECG (the app collects that type of data) and now the test passes again.

In addition I improved a few things that I saw here and there with the following comments:

The UI tests should now pass and you should be able to merge the branch in the main branch once the coverage report comes that will now include the reenabled UI tests.

@PSchmiedmayer
Copy link
Member

@ckunchur Short update here: There was a problem related to git LFS (similar in origin to https://github.com/orgs/CS342/discussions/29, but not the same) that I have addressed in 311574d.
To make the tests a bit more reliable, I have also increased the timeout here a bit: 221b0bc

@ckunchur ckunchur merged commit 565f26b into main Feb 16, 2023
@ckunchur ckunchur deleted the caitlin branch February 16, 2023 00:48
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