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

Auct1037 rerequest email confirmation #5728

Merged
merged 4 commits into from
Jun 5, 2020

Conversation

erikdstock
Copy link
Contributor

@erikdstock erikdstock commented Jun 4, 2020

Jira Ticket AUCT-1037
This PR duplicates #5667 by changing the static 'link expired' message to one which includes a mutation for re-requesting email confirmation. It is mostly copy-pasted over with a new mutation and analytics schema value.

Notably this banner, when triggered by ?flash_message=expired_token will appear whether you can request email confirmation (in a me.canRequestEmailConfirmation sense) or not.

Including the mobile view as an edge case:
image

TODO:

  • Take a stab at extracting the common email confirmation request component
  • Add type signature to the selectContent function

@erikdstock erikdstock force-pushed the auct1037-rerequest-email-conf branch from f0f85f1 to dddd50b Compare June 4, 2020 18:45
expect(wrapper.text()).toContain("Link expired.")
})

it.only("user seeing banner can click to re-trigger email confirmation message", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

stray only

queryString: "?flash_message=expired_token",
})

expect(wrapper.text()).toContain("Link expired.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assert upon the larger text that's now displayed. namely "Resend verification email"


expect(wrapper.text()).toContain("Link expired.")

wrapper.find("button").first().prop("onClick")({} as any)
Copy link
Contributor

Choose a reason for hiding this comment

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

simulate("click")

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'll make these changes bc i prefer them too but the main reason i changed it is that the Enzyme team has effectively marked it for deprecation - essentially simulate('action', event) is just syntactic sugar for prop('onAction')(event) and so this api has created the misleading impression that key features like event bubbling will be simulated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting - I wasn't aware of this! Thank you for clarifying – I read the issue and looks like there's a fair bit of back and forth on the semantics of shallow rendering vs. full DOM rendering with respect to the simulate API.

Regardless, I think, on principle, being consistent is more important than preparing for this API to be deprecated. A find+replace across the codebase when the API does indeed become deprecated should be pretty straightfoward.

.find("button")
.first()
.prop("onClick")({} as any)
wrapper.find("button").first().prop("onClick")({} as any)
Copy link
Contributor

Choose a reason for hiding this comment

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

simulate("click")

.find("button")
.first()
.prop("onClick")({} as any)
wrapper.find("button").first().prop("onClick")({} as any)
Copy link
Contributor

Choose a reason for hiding this comment

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

simulate("click")


expect(wrapper.text()).toContain("Link expired.")

wrapper.find("button").first().prop("onClick")({} as any)
Copy link
Contributor

Choose a reason for hiding this comment

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

simulate("click")

Copy link
Contributor Author

@erikdstock erikdstock left a comment

Choose a reason for hiding this comment

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

updated addressing all feedback- thank you!


expect(wrapper.text()).toContain("Link expired.")

wrapper.find("button").first().prop("onClick")({} as any)
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'll make these changes bc i prefer them too but the main reason i changed it is that the Enzyme team has effectively marked it for deprecation - essentially simulate('action', event) is just syntactic sugar for prop('onAction')(event) and so this api has created the misleading impression that key features like event bubbling will be simulated.

@erikdstock erikdstock merged commit 525815a into artsy:master Jun 5, 2020
@artsy-peril artsy-peril bot mentioned this pull request Jun 5, 2020
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.

3 participants