feat: implement new login and "connect project" logic #23762
Conversation
There was a problem hiding this comment.
@marktnoonan Still going through items, but wanted to post this since you were still working in this area. I hope my suggestions with using reactive in SpecsListBanners simplifies things.
| @@ -116,6 +116,10 @@ describe('App: Runs', { viewportWidth: 1200 }, () => { | |||
| obj.result.data.cloudViewer.organizations.nodes = [] | |||
| } | |||
|
|
|||
| if (obj.result.data?.cloudViewer?.firstOrganization?.nodes) { | |||
| obj.result.data.cloudViewer.firstOrganization.nodes = [] | |||
There was a problem hiding this comment.
I know that the need for firstOrganization was introduced in previous work, but I hope we can take some more time soon to find out why we have to rely on this trick while we also have the organizations being returned at the same time.
| @@ -181,7 +191,7 @@ describe('App: Runs', { viewportWidth: 1200 }, () => { | |||
| cy.startAppServer('component') | |||
|
|
|||
| cy.remoteGraphQLIntercept(async (obj, testState) => { | |||
| if (obj.operationName === 'CloudConnectModals_RefreshCloudViewer_refreshCloudViewer_cloudViewer') { | |||
| if (obj.operationName === 'LoginConnectModals_LoginConnectModalsQuery_cloudViewer') { | |||
There was a problem hiding this comment.
This change was just updating existing code, but looking at the rest of the code base, this spec file is the only place that there is conditional logic based on operationName. This is a fragile pattern to have look at this string. This is definitely a code smell to me but I have not looked at what the alternative would be yet.
There was a problem hiding this comment.
These mutation names do get into a lot of places, and since their naming convention depends on our component architecture, we end up expressing that architecture in our tests, which yeah does feel a bit off, but also not a huge burden in practice imo, since we rarely change the architecture.
| return { | ||
| component: componentsByStatus[loginConnectStore.userStatus] ?? null, | ||
| cohortOption: getCohortForBanner(bannerIds[loginConnectStore.userStatus]), | ||
| } |
There was a problem hiding this comment.
| return { | |
| component: componentsByStatus[loginConnectStore.userStatus] ?? null, | |
| cohortOption: getCohortForBanner(bannerIds[loginConnectStore.userStatus]), | |
| } | |
| return reactive({ | |
| component: componentsByStatus[loginConnectStore.userStatus] ?? null, | |
| cohortOption: getCohortForBanner(bannerIds[loginConnectStore.userStatus]), | |
| }) |
Wrapping the return in reactive should allow for the un-nesting of the refs but still allow them to be reactive. That way, all the nested .value calls can be removed. I have not fully tested this yet, but I believe this should work.
There was a problem hiding this comment.
nice, I will check that out, I had drafted a comment about this but your review beat me to it 🙏
| @@ -294,6 +268,10 @@ const bannerCohortOptions = { | |||
| const cohortBuilder = useCohorts() | |||
|
|
|||
| const getCohortForBanner = (bannerId: string) => { | |||
| if (!bannerCohortOptions[bannerIds[loginConnectStore.userStatus]]) { | |||
There was a problem hiding this comment.
| if (!bannerCohortOptions[bannerIds[loginConnectStore.userStatus]]) { | |
| if (!bannerCohortOptions[bannerId]) { |
packages/frontend-shared/src/gql-components/modals/SelectCloudProjectModal.vue
Show resolved
Hide resolved
packages/frontend-shared/src/gql-components/LoginConnectModalsContent.vue
Show resolved
Hide resolved
packages/frontend-shared/src/gql-components/modals/SelectCloudProjectModal.vue
Show resolved
Hide resolved
| watch(() => loginConnectStore.isLoginConnectOpen, (newVal) => { | ||
| if (newVal) { | ||
| executeQuery() | ||
| } | ||
| }, { | ||
| immediate: true, | ||
| }) | ||
|
|
||
| const executeQuery = async () => { | ||
| await query.executeQuery() | ||
| } |
There was a problem hiding this comment.
| watch(() => loginConnectStore.isLoginConnectOpen, (newVal) => { | |
| if (newVal) { | |
| executeQuery() | |
| } | |
| }, { | |
| immediate: true, | |
| }) | |
| const executeQuery = async () => { | |
| await query.executeQuery() | |
| } | |
| const executeQuery = async () => { | |
| await query.executeQuery() | |
| } | |
| whenever(() => loginConnectStore.isLoginConnectOpen, executeQuery) |
Short and sweet? Didn't test this though
There was a problem hiding this comment.
I like this, updated.
There was a problem hiding this comment.
Nice! I did not know about whenever. I need to spend more time looking through the vueUse library!
| @@ -159,3 +162,22 @@ async function fileExists (absolute: string) { | |||
| return false | |||
| } | |||
| } | |||
|
|
|||
| export async function hasNonExampleSpec (testTemplateDir: string, specs: string[]): Promise<boolean> { | |||
There was a problem hiding this comment.
This code feels like it doesn't belong here and would make more sense in ProjectActions
There was a problem hiding this comment.
I think @warrensplayer had a reason for keeping it here & calling it from ProjectActions, maybe he can refresh my memory?
There was a problem hiding this comment.
@ZachJW34 @marktnoonan I felt a little weird about putting it there, but kept it in this file because I was using internal methods (allFilesInDir and fileExists) that I did not want to refactor at the time I created that method.
There was a problem hiding this comment.
I'm fine with where it is then, could do with a comment saying a refactor/move would be good.
There was a problem hiding this comment.
Overall, the code is great. The part I love most is all the deletion of code! That was a great update to SpecsListBanners to consolidate the creation of the cohort with the determination of the banner to show.
Only one strange thing I am seeing during testing and not sure if it is an existing issue or not. Logging in with the menu item in the header seems fine. However, using the tooltip or the LoginBanner, sometimes causes things to get "stuck". I have attached a screen recording to show one example. In this example, one item has the "loading" skeleton stuck showing. Also, when logging back out, the LoginBanner does not reappear. Not sure what is happening here yet.
Screen.Recording.2022-10-21.at.3.16.28.PM.mov
| cy.get(`[data-cy="${bannerTestId}"]`).should('be.visible') | ||
| }) | ||
|
|
||
| it('should be preempted by spec not found banner', () => { | ||
| mountWithState(gql, stateWithFirstOpenedDaysAgo(4), { isSpecNotFound: true }) | ||
| mountWithState(gql, undefined, { isSpecNotFound: true }) |
There was a problem hiding this comment.
Can the undefined be an empty object instead? {}
|
|
||
| _.merge(this.ctx.coreData.localSettings.preferences, toJson) |
There was a problem hiding this comment.
I see that LocalSettingsActions does not have any unit tests. Can you see if it would be easy to add a unit test for the method this is in?
There was a problem hiding this comment.
Just commenting for the record that these have been added!
| watch(() => loginConnectStore.isLoginConnectOpen, (newVal) => { | ||
| if (newVal) { | ||
| executeQuery() | ||
| } | ||
| }, { | ||
| immediate: true, | ||
| }) | ||
|
|
||
| const executeQuery = async () => { | ||
| await query.executeQuery() | ||
| } |
There was a problem hiding this comment.
Nice! I did not know about whenever. I need to spend more time looking through the vueUse library!
| component: componentsByStatus[loginConnectStore.userStatus] ?? null, | ||
| cohortOption: getCohortForBanner(bannerIds[loginConnectStore.userStatus]), | ||
| } | ||
| return componentsByStatus[loginConnectStore.userStatus] ?? null |
There was a problem hiding this comment.
Any particular reason you split the cohortOption out of this method again? I liked that the bannerToShow encapsulated both the banner and the props it needed, but I do not think it is worth refactoring again. It works this way as well.
There was a problem hiding this comment.
I liked it too, but it turned out Vue was logging warnings about wrapping the components in a reactive object. I don't recall the exact warning, but splitting them up seemed the best way to get what we wanted, at least for now.
|
@ZachJW34 I think I responded to your comments, let me know if there's anything else that sticks out. |
Co-authored with @warrensplayer
There are lots of changes in this PR, though many of the are minor changes that come from using the new Login Connect store, moving files around, and simplifying some component APIs.
This PR:
Allowing users to connect projects from the Launchpad login flow required moving many components and queries into frontend-shared, which had various implications for tests.
Developer facing improvements
Through incremental changes introduced since Cypress 10.0.0, the implementation for ways to trigger modals and track state related to logging in and connecting projects has become unwieldy, with similar ideas implemented in multiple places, in slightly different ways. The complexity of the current implementation exceeds the complexity of the underlying ideas, and this PR attempts to bring them closer together by using the logic merged in #24101 to drive the UI.
modelValuebindings where we were not leveragingv-modelbehavior.setPreferencesaction in data-context will now do a lodashmergeinstead of a simple loop resetting properties that were passed to it, this avoids some reading/merging of deep preferences that we were having to do on the client now that we have nested values in there likebanners, which was causing a problem in this PR as we'd step on freshly saved data with some other new sibling data.Further improvements
We could always go further with refactoring, but had to draw the line somewhere to merge this. Things this PR doesn't address:
firstOrganizationin this PR) that aren't used directly by the component but need to stay up to date, and if left out can cause problems - this feels like a bit of a bad pattern, it's easy to remove something that's not obviously being used, only to find out that it can throw off some other component.setUpStatusfunction fromisAllowedFeature.cy.ts, if all of our tests use this, it will simplify them and they won't need to know details of what values in the store lead to each state.User facing changelog
Features
Minor Bugfixes
Additional details
Steps to test
Launchpad - Login/Connect/Record flow
Loom video of expectations for these steps in the launchpad
Global Mode
With any project open
With a project that has no project id
With a project that has a project id, but no recorded runs in the dashboard
App - Login/Connect/Record flow
With an unconnected project open (no project ID in config) (loom video)
promptsShown.loginModalRecordfrom state.json for this project if it is presentWith a connected project that has no recorded runs (Loom video]
promptsShown.loginModalRecordfrom state.json for this project if it is presentpromptsShown.loginModalRecordshould exist in state.json for this projectWith a connected project that has recorded runs
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?