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

PWA-138: Re-factor getUserDetails to use GraphQL (useAwaitQuery Edition) #2004

Merged
merged 7 commits into from
Dec 3, 2019

Conversation

tjwiebell
Copy link
Contributor

@tjwiebell tjwiebell commented Nov 25, 2019

Description

See PWA-138

Related Issue

  • [PWA-138] User details should be fetched from GraphQL

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Create Account
  • Go through Create Account workflow
  • Verify user details are displayed correctly after successful creation (at bottom on nav bar)
  1. Sign-In
  • Logout and sign-in with the same account as above
  • Verify you're redirected to MyAccount/Sign-out Page
  • Verify user details still displayed correctly
  1. Sign-out
  • Sign-out
  • Verify user details is replaced by signin button
  1. MyAccount
  • While signed in, clear browser cache while retaining local storage
  • Verify user details are loading correctly on initial app render

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Nov 25, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-138.

Generated by 🚫 dangerJS against 2d6c798

const userDetails = await request('/rest/V1/customers/me', {
method: 'GET'
});
const { data } = await fetch();
Copy link
Contributor

Choose a reason for hiding this comment

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

This prevents other actions from calling each other as only components will have fetchers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we would want to ensure we handle the error appropriately. Does fetch throw an error or would we have to destructure error from the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...you're absolutely right, and it would be nasty to have to pass query invokers for nested action calls, but it would more clearly define those action APIs.

fetch does throw on error, so this code should function as-is (though may need to destructure { error } in the catch.

@tjwiebell tjwiebell changed the title [SPIKE] Async Data Fetching in Thunks Re-factor getUserDetails to use GraphQL (useAwaitQuery Edition) Nov 26, 2019
@tjwiebell tjwiebell added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Nov 26, 2019
@tjwiebell tjwiebell marked this pull request as ready for review November 26, 2019 17:24
@tjwiebell tjwiebell changed the title Re-factor getUserDetails to use GraphQL (useAwaitQuery Edition) PWA-138: Re-factor getUserDetails to use GraphQL (useAwaitQuery Edition) Nov 27, 2019
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

This does look cleaner. Please add some docs, otherwise 👍

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

<3

@supernova-at
Copy link
Contributor

supernova-at commented Dec 2, 2019

QA

  • Verification Steps Pass
  • Tested in Chrome
  • Tested in Safari
  • Tested on iOS simulator
  • MFTF Tests Pass

method: 'GET'
const { data } = await fetchUserDetails({
// until we can investigate some odd behavior with apollo-cache-persist
// not busting the cache on sign out, avoid caching user details.
Copy link
Contributor

Choose a reason for hiding this comment

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

@tjwiebell did we create a ticket for this?

@supernova-at supernova-at merged commit 2dff7a7 into develop Dec 3, 2019
@supernova-at supernova-at deleted the tommy/use-await-query branch December 3, 2019 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants