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

Orion signup flow rework #321

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

zeeshanakram3
Copy link
Contributor

Addresses #320

id: ID!

"Membership associated with the blockchain account (controllerAccount)"
memberships: [Membership!] @derivedFrom(field: "controllerAccount")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support multiple memberships within Atlas? Seems, like an overkill. Besides that it is possible from runtime perspective, it will only complicate the membership dropdown (we already had some feedback that it is complicated @dmtrjsg)

{ joystreamAccountId: payload.joystreamAccountId }
)

console.log('after update')
Copy link
Contributor

Choose a reason for hiding this comment

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

log

})

// Mark the token as used
token.expiry = new Date()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just drop the token if used?

@@ -20,33 +31,102 @@ export class AccountResolver {
const account = ctx.account
const em = await this.em()
assert(account, 'Unexpected context: account is not set')
const { id, email, joystreamAccount, membershipId, isEmailConfirmed, notificationPreferences } =
account
const { id, email, joystreamAccount, notificationPreferences } = account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const { id, email, joystreamAccount, notificationPreferences } = account
const { id, email, joystreamAccountId, notificationPreferences } = account

In authenticate we don't fetch the account with BlockchainAccount relation, so joystreamAccount is empty.

Rn I adjusted the Atlas to use joystreamAccountId, but I can switch it, if we decide to include the relation.


@Field(() => String, { nullable: false })
membershipId: string
joystreamAccount!: BlockchainAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we will have to either introduce new type-graphql class or go with joystreamAccountId approach.

Plus lets make it nullable, since there is a possibilty that user creates an account, but membership creation fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if the membership creation fails, the joystreamAccountId will still not be null, since it is provided in the create account request


type ReqParams = Record<string, string>
type ResBody =
| components['schemas']['GenericOkResponseData']
| components['schemas']['GenericErrorResponseData']
type ReqBody = components['schemas']['RequestTokenRequestData']
type ResLocals = { authContext: Session }

export const requestEmailConfirmationToken: (
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont know how, but I managed to generate 2 email tokens and orion now returns the first one, which turns out is expired as I try to make an account.

image

@ikprk
Copy link
Contributor

ikprk commented Jun 9, 2024

we need to add even more details to email confirmation link

we would need 3 different signup-type: internal, external and ypp, if the type is ypp we would need url of the video to verify it again to get user-related data

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.

None yet

3 participants