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

feat(passport): keep connected account selection between signins #2363

Merged
merged 22 commits into from
Jun 12, 2023

Conversation

poolsar42
Copy link
Collaborator

@poolsar42 poolsar42 commented Jun 5, 2023

Description

Displays connected emails as connected accounts (it's a fix)
And also redesigns authz screen to display previously connected emails and sc wallets

Related Issues

Testing

Storybook and authz route

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@poolsar42 poolsar42 self-assigned this Jun 5, 2023
@poolsar42 poolsar42 force-pushed the feat/2320-passport-keep-connected-selection branch from e3f0546 to 5759730 Compare June 5, 2023 23:46
@poolsar42 poolsar42 marked this pull request as ready for review June 5, 2023 23:48
@poolsar42 poolsar42 force-pushed the feat/2320-passport-keep-connected-selection branch from c57c6da to 3cc1739 Compare June 7, 2023 04:24
Copy link
Contributor

@betimshahini betimshahini left a comment

Choose a reason for hiding this comment

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

Expand your test cases in the PR description.


let minDate = filteredEmailsFromConnectedAddresses[0].createdTimestamp!

if (preselected) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite following what this is supposed to be doing; the edges call returns items ordered by created timestamp already. Can you not just pick the first/last, as needed?

Copy link
Collaborator Author

@poolsar42 poolsar42 Jun 9, 2023

Choose a reason for hiding this comment

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

Per our conversation in chatted I assumed we wanted to double check it here. If it's not needed - removing it
done ☑️

import { getEmailIcon } from '@proofzero/utils/getNormalisedConnectedAccounts'
import { ThemeContext } from '@proofzero/design-system/src/contexts/theme'
import { AuthenticationScreenDefaults } from '@proofzero/design-system/src/templates/authentication/Authentication'
import { Helmet } from 'react-helmet'
import { getRGBColor } from '@proofzero/design-system/src/helpers'
import { AddressURNSpace } from '@proofzero/urns/address'
import type { DropdownSelectListItem } from '@proofzero/design-system/src/atoms/dropdown/DropdownSelectList'
Copy link
Contributor

Choose a reason for hiding this comment

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

Importer complaining about this import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-09 at 16 16 20 Don't see it on my end. What does it say?

form.append('createSCWallet', JSON.stringify({
check: true,
nickname: appProfile.name
}))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing the retrieval and pre-selection of values from an existing authorization, if available (ticket #2320). Has that been implemented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, then here, here and here and down to two other places

@poolsar42 poolsar42 force-pushed the feat/2320-passport-keep-connected-selection branch from 9f80d5c to e9123d6 Compare June 9, 2023 20:14
@poolsar42 poolsar42 force-pushed the feat/2320-passport-keep-connected-selection branch from e9123d6 to 9f9a4cb Compare June 12, 2023 15:39
request,
params: { clientId },
context,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we need non dynamic scopes

context.traceSpan
)

const { addressURN } = await addressClient.initSmartContractWallet.query({
Copy link
Contributor

Choose a reason for hiding this comment

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

why not import the actionLoader from the current create wallet route and pass in params?

Copy link
Contributor

Choose a reason for hiding this comment

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

or create a helper functino that both acrtion loaders can use

export const getDataForScopes = async (
requestedScope: string[],
accountURN: AccountURN,
jwt?: string,
env?: any,
traceSpan?: any
traceSpan?: any,
preauthorizedScopes?: GetAuthorizedAppScopesMethodResult['claimValues']
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we need any of this new stuff. we can just mix in the persona data to select the values on the client

}
return addressType.charAt(0).toUpperCase() + addressType.slice(1)
}

export const getEmailDropdownItems = (
connectedAddresses?: Addresses | null
connectedAddresses?: Addresses | null,
preselected = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

try to defer to the client to handle to select them

@poolsar42 poolsar42 force-pushed the feat/2320-passport-keep-connected-selection branch from ae20939 to 7a69957 Compare June 12, 2023 18:29
@maurerbot maurerbot merged commit a367741 into main Jun 12, 2023
@maurerbot maurerbot deleted the feat/2320-passport-keep-connected-selection branch June 12, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants