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

LOOP-887 Keycloak updates #84

Merged
merged 44 commits into from
May 3, 2023
Merged

LOOP-887 Keycloak updates #84

merged 44 commits into from
May 3, 2023

Conversation

ps2
Copy link
Contributor

@ps2 ps2 commented Apr 21, 2023

https://tidepool.atlassian.net/browse/LOOP-887

Updates for OAuth2 based authorization. Also brings in some translations from DIY (not used yet in Tidepool Loop), and a stubbed out RemoteCommand method required for Service protocol updates, also not used in Tidepool Loop.

ps2 and others added 30 commits January 20, 2023 16:28
Updated translations from Lokalise on Thu Feb  9 13:30:18 CST 2023
…-service

Remote PR Set #2: Introduce RemoteCommands
…-command-service

Revert "Remote PR Set #2: Introduce RemoteCommands"
Upload automation status for temp and normal basals.
Updated translations from Lokalise on Sat Mar 18 15:11:44 CDT 2023
Basal rate fixes, and tests passing.
ps2 added 10 commits March 24, 2023 12:58
* Updating to new TidepoolKit with keycloak based auth

* SettingsView using TidepoolService as ObservedObject

* Tweak logo size

* Fix issues with state restoration and re-logging in. Add alert when session loss is detected

* Improve DataSetId caching

* Do not allow environment switching when logged in

* Tweak wording
@ps2 ps2 requested review from nhamming and Camji55 April 24, 2023 15:57
Comment on lines 32 to 38
// var datumOrigin: TOrigin {
// return datumOrigin(for: resolvedIdentifier)
// }
//
// func datumOrigin<T: TypedDatum>(for type: T.Type) -> TOrigin {
// return datumOrigin(for: resolvedIdentifier(for: type))
// }
Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed?

Comment on lines 33 to 39
// var datumOrigin: TOrigin? {
// return datumOrigin(for: resolvedIdentifier)
// }
//
// func datumOrigin<T: TypedDatum>(for type: T.Type) -> TOrigin? {
// return datumOrigin(for: resolvedIdentifier(for: type))
// }
Copy link
Member

Choose a reason for hiding this comment

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

This too?

// TODO: This implementation is incorrect and will not record the correct history when data is updated. Currently waiting on
// https://tidepool.atlassian.net/browse/BACK-815 for backend to support new API to capture full history of data changes.
// This work will be covered in https://tidepool.atlassian.net/browse/LOOP-3943. For now just call createData with the
// updated data as it will just overwrite the previous data with the updated data.
tapi.createData(data, dataSetId: dataSetId) { error in
Copy link
Member

Choose a reason for hiding this comment

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

Is the above comment still relevant after these changes?

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 believe this is still outstanding, but isn't a blocker, as the creation makes sure the latest data is uploaded, but we lose some history.

import TidepoolKit
import TidepoolServiceKit

@MainActor
Copy link
Member

Choose a reason for hiding this comment

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

I think View will automatically be on the MainActor if it contains an ObservableObject

import TidepoolKit
import TidepoolServiceKit

@MainActor
Copy link
Member

Choose a reason for hiding this comment

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

I think View will automatically be on the MainActor if it contains an ObservableObject. Was there a specific need for this annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're correct. I removed this and purposefully made the login function (called from loginButtonTapped) return from a non-main thread, and it switched back over to main actor.

do {
environments = try await TEnvironment.fetchEnvironments()
} catch {

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to write to the error property here?

Suggested change
self.error = error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the environments is a "secret" list, that shouldn't expose errors to the user on failure. But good question.

do {
try await login?(selectedEnvironment)
isLoggingIn = false
//dismiss?()
Copy link
Member

Choose a reason for hiding this comment

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

dismiss needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, will remove.

try await service.tapi.login(environment: environment, presenting: navController)
try await service.completeCreate()
await navController.notifyServiceCreatedAndOnboarded(service)
//await navController.notifyComplete()
Copy link
Member

Choose a reason for hiding this comment

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

Needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, will remove.

Copy link
Contributor

@nhamming nhamming left a comment

Choose a reason for hiding this comment

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

LGTM

@ps2 ps2 merged commit 837924d into dev May 3, 2023
@ps2 ps2 deleted the ps2/LOOP-887/keycloak-updates branch May 3, 2023 23:20
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.

4 participants