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

Configure HealthKit and notification authorizations #6

Merged
merged 16 commits into from
Jan 23, 2024
Merged

Conversation

dhruvna1k
Copy link
Contributor

@dhruvna1k dhruvna1k commented Jan 17, 2024

Configure HealthKit and notification authorizations

♻️ Current situation & Problem

The data being collected from HealthKit in the TemplateApplication is not configured for our purposes. There is also pre-existing notification permissions, which we do not need.

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
No additional documentation needed, just a configuration of the code

✅ Testing

No additional testing needed, code and previews compile

📝 Code of Conduct & Contributing Guidelines

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

@dhruvna1k dhruvna1k enabled auto-merge (squash) January 17, 2024 01:38
@dhruvna1k dhruvna1k disabled auto-merge January 17, 2024 01:39
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (1d168bf) 38.12% compared to head (ab14931) 40.90%.
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   38.12%   40.90%   +2.79%     
==========================================
  Files          31       30       -1     
  Lines         976      961      -15     
==========================================
+ Hits          372      393      +21     
+ Misses        604      568      -36     
Files Coverage Δ
Behavior/BehaviorDelegate.swift 94.92% <100.00%> (+0.09%) ⬆️
Behavior/Onboarding/OnboardingFlow.swift 96.56% <100.00%> (-0.67%) ⬇️
Behavior/Schedule/BehaviorScheduler.swift 83.88% <100.00%> (+1.73%) ⬆️
Behavior/Contacts/Contacts.swift 71.96% <82.36%> (+2.57%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@PSchmiedmayer PSchmiedmayer self-requested a review January 17, 2024 07:36
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 @dhruvna1k,

Thank you for the PR and the improvements. It's a good first version that needs some more work to be merged.

First: Please update your GitHub profile to show your full name and a profile picture!

The PR in its current state is a bit incomplete: It does not update the text in the HealthKit onboarding flow to change the text to the needs of your application. Please complete this before re-requesting a review.

I have noticed that your SwiftLint checks are failing due to some changes in the code that do not conform to the SwiftLint style guide. Please be sure to address them. You can learn more about each rule, including passing and failing examples, in the Swift Documentation.

In addition, your PR description does not follow the PR template that we introduced in the class, and that is provided by default: https://github.com/CS342/.github/blob/main/.github/pull_request_template.md.
Please follow the following template in your PR description and add a good description of your PR by editing the PR description on the PR page.
Please also add a more descriptive and clear title focusing on the added functionality rather than the assignment task.

Please also read through the "Keeping your pull request in sync with the base branch" documentation to ensure that your branch is up to date with the main branch before you merge it into the main branch.

Please re-request a review once you have implemented all the requested changes.

Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/Onboarding/OnboardingFlow.swift Outdated Show resolved Hide resolved
Behavior/Onboarding/OnboardingFlow.swift Show resolved Hide resolved
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Jan 18, 2024
@dhruvna1k dhruvna1k changed the title task 3 Configure HealthKit and notification authorizations Jan 21, 2024
@dhruvna1k dhruvna1k enabled auto-merge (squash) January 21, 2024 17:51
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 @dhruvna1k,

Thank you for the improvements in the PR!

Thank you for adding the profile picture: Please also update your GitHub profile to show your full name.

Please also read through the "Keeping your pull request in sync with the base branch" documentation to ensure that your branch is up to date with the main branch before you merge it into the main branch.

Please re-request a review once you have implemented all the requested changes.

Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
Behavior/Resources/Localizable.xcstrings Outdated Show resolved Hide resolved
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.

@dhruvna1k We are getting closer to a finished PR.

Please be sure that you run and test the application in Xcode and that you resolve all the conversations completely by addressing them in the code before re-requesting a review.

Once the last conversations are resolved please re-request a review 👍

Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
Behavior/BehaviorDelegate.swift Outdated Show resolved Hide resolved
@dhruvna1k
Copy link
Contributor Author

@dhruvna1k We are getting closer to a finished PR.

Please be sure that you run and test the application in Xcode and that you resolve all the conversations completely by addressing them in the code before re-requesting a review.

Once the last conversations are resolved please re-request a review 👍

Thanks for the reminder. I think I forgot to build the project before commiting/pushing the changes last time. Have made the proper changes and remembered to build this time before committing/pushing. Let me know if this doesn't work.

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.

@dhruvna1k Thank you for the improvements and fixing the last issues! The PR looks great to me and good job with improving it step by step.

We hope you learned some new stills in the PR and are looking forward to a great project this quarter!

@dhruvna1k dhruvna1k merged commit 7ab866e into main Jan 23, 2024
7 checks passed
@dhruvna1k dhruvna1k deleted the a1/t3/dhruv branch January 23, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants