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

ABW-1872 Transition to new persona data format/rewire persona from dapp details #611

Conversation

maciek-rdx
Copy link
Contributor

@maciek-rdx maciek-rdx commented Jul 11, 2023

Jira ticket: ABW-1872

Description

Fixes the crash that you can experience when opening Persona details from dApp Details screen.

How to test

Happy path (or test variation 1)

  1. Link with dapp, create persona
  2. Go to Settings/Authorized dApps//

The app should not crash and display the right data.

Screenshot

IMG_0021

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

@maciek-rdx maciek-rdx requested review from kugel3 and removed request for kugel3 July 11, 2023 15:55
fatalError()
guard case let .dApp(dApp, persona) = mode else { return nil }
return .init(
name: dApp.displayName?.rawValue ?? L10n.DAppRequest.Metadata.unknownName,
Copy link
Contributor

Choose a reason for hiding this comment

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

in general I think default values like "unknown" should be added as late as possible, ideally in the viewstate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, this IS the viewstate, then that's all good.

@@ -127,8 +127,8 @@ public struct PersonasCoordinator: Sendable, FeatureReducer {
return .task {
let dApps = try await authorizedDappsClient.getDappsAuthorizedByPersona(persona.id)
.map(PersonaDetails.State.DappInfo.init)
let personaDetails = PersonaDetails.State(.general(persona, dApps: .init(uniqueElements: dApps)))
return .internal(.loadedPersonaDetails(personaDetails))
let personaDetailsState = PersonaDetails.State(.general(persona, dApps: .init(uniqueElements: dApps)))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't typically add "state" at the end of variables. Note also that the name of the case is loadedPersonaDetails.

Copy link
Contributor

@kugel3 kugel3 left a comment

Choose a reason for hiding this comment

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

I don't think we need to make the variable names longer though.

@maciek-rdx maciek-rdx merged commit 225c20e into main Jul 12, 2023
3 checks passed
@maciek-rdx maciek-rdx deleted the ABW-1872-transition-to-new-persona-data-format/rewire-persona-from-dapp-details branch July 12, 2023 09:37
@kugel3
Copy link
Contributor

kugel3 commented Jul 12, 2023

@maciek-rdx We should NOT use "personaDetailsState", it should be "personaDetails".

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants