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: artwork page - link to user idv flow if applicable #6366

Merged
merged 3 commits into from
Oct 1, 2020

Conversation

erikdstock
Copy link
Contributor

@erikdstock erikdstock commented Sep 29, 2020

This PR adds the link-to-identity-verification functionality similar to #6342 to our artwork pages, prompting a user with a pending identity verification to complete it. The affected component contains logic for several different sale types but i have only added a test for one of them - something probably worth addressing. There is also some basically duplicated logic between this and Registration.tsx, which is for the sale page and does not use relay.

image

@erikdstock erikdstock self-assigned this Sep 29, 2020
@erikdstock erikdstock requested a review from bhoggard September 29, 2020 20:57
@@ -18,7 +18,6 @@ import { ArtworkSidebarBidAction_me } from "v2/__generated__/ArtworkSidebarBidAc
import * as Schema from "v2/Artsy/Analytics/Schema"
import track from "react-tracking"
import { getENV } from "v2/Utils/getENV"
import { bidderNeedsIdentityVerification } from "v2/Utils/identityVerificationRequirements"
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 have now factored this out of two files (the other being Registration.tsx) because we wanted more granular definitions of what to show a user something to consider.

Comment on lines 138 to 145
const userLacksIdentityVerification =
sale.requireIdentityVerification && !me?.identityVerified
const pendingIdentityVerification = me?.pendingIdentityVerification

const shouldPromptIdVerification =
!qualifiedForBidding &&
userLacksIdentityVerification &&
Boolean(pendingIdentityVerification)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the function i have been removing should actually be updated to return an interface full of idv-related properties like this instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, as long as the login remains confined to this component.

@admbtlr
Copy link
Contributor

admbtlr commented Sep 30, 2020

Thanks for using semantic style of the PR title! 🙌 Strictly speaking, there isn't an enhancement option, so maybe just go with feat?

@erikdstock erikdstock changed the title enhancement: artwork page - link to user idv flow if applicable feat: artwork page - link to user idv flow if applicable Sep 30, 2020
@erikdstock
Copy link
Contributor Author

@bhoggard, this is now complete and ready for squash/merge.

@erikdstock
Copy link
Contributor Author

#squashongreen

@artsy-peril artsy-peril bot added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Oct 1, 2020
@artsy-peril artsy-peril bot merged commit 0a80c00 into artsy:master Oct 1, 2020
@artsy-peril artsy-peril bot mentioned this pull request Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants