Skip to content

[dashboard] fix onboarding modal show with workspaces exists #9665

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

Closed
wants to merge 1 commit into from

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Apr 29, 2022

Description

Go with origin proposal, onboarding Modal in /workspaces page will only show with

  • User is onboarding user(not select IDE yet)
  • No workspaces in /workspaces page

See also #9624 (comment)

Related Issue(s)

Fixes #

How to test

  1. Go to /workspaces verify that the IDE selection modal is visible, do not interact with it, just close the browser tab
  2. Launch a new workspace via /#<repo_url>
  3. Go to /workspaces verify that the modal does not show
  4. Delete your workspace created by step 2
  5. Go back to /workspaces and verify that the modal is showing again

Release Notes

NONE

Documentation

@mustard-mh mustard-mh requested a review from a team April 29, 2022 21:01
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 29, 2022
@andreafalzetti
Copy link
Contributor

andreafalzetti commented Apr 29, 2022

Thanks for raising the PR 🙏

@mustard-mh
Copy link
Contributor Author

@andreafalzetti Any idea what is workspaceModel?.searchTerm? Maybe it don't need to be a condition of <SelectIDEModal>

@andreafalzetti
Copy link
Contributor

andreafalzetti commented Apr 29, 2022

@andreafalzetti Any idea what is workspaceModel?.searchTerm? Maybe it don't need to be a condition of <SelectIDEModal>

I think it's the state for this search bar:

Screenshot 2022-04-29 at 23 26 30

The search bar it's only visible when you have workspaces

@mustard-mh mustard-mh force-pushed the hw/fix-dashboard-onboarding-bug branch from 432288b to b9e3e67 Compare April 29, 2022 21:31
@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 3, 2022

/werft run

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.2
(with .werft/ from main)

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 3, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.3
(with .werft/ from main)

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 3, 2022

/werft run without-vm=true

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.4
(with .werft/ from main)

Copy link
Contributor

@andreafalzetti andreafalzetti left a comment

Choose a reason for hiding this comment

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

LGTM

Tested, works as expected! 🚀

@laushinka
Copy link
Contributor

laushinka commented May 6, 2022

Taking a look! @mustard-mh @andreafalzetti Should I run this without-vm?

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 6, 2022

Taking a look! @mustard-mh @andreafalzetti Should I run this without-vm?

I think for testing it works either way. The reason I've used without-vm is because with the changes to preview environment I was having issues connecting to the db with mysql-cli since the new approach required to SSH into the vm and I didn't know.

@mustard-mh
Copy link
Contributor Author

mustard-mh commented May 6, 2022

FYI. Access VM preview environment k3s:

  • Switch to correct branch i.e hw/fix-dashboard-onboarding-bug
  • Exec ./dev/preview/install-k3s-kubeconfig.sh

Then, all steps (exec \ port-forward etc) like before

cc @andreafalzetti @laushinka

@mustard-mh
Copy link
Contributor Author

mustard-mh commented May 6, 2022

/werft run

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.5
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented May 6, 2022

/werft run

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.6
(with .werft/ from main)

@laushinka laushinka self-assigned this May 6, 2022
@laushinka
Copy link
Contributor

laushinka commented May 6, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.7
(with .werft/ from main)

@laushinka
Copy link
Contributor

laushinka commented May 6, 2022

@mustard-mh I'm unable to access the preview env for some reason 🤔

@mustard-mh
Copy link
Contributor Author

mustard-mh commented May 6, 2022

/werft run

👍 started the job as gitpod-build-hw-fix-dashboard-onboarding-bug.8
(with .werft/ from main)

@mustard-mh
Copy link
Contributor Author

mustard-mh commented May 6, 2022

This PR has not committed for a long time, so platform team's job will delete this harvester(VM) preview environment every 30 mins 🥲 cc @laushinka @andreafalzetti

cron: "15 * * * *"

Every xx:15 https://github.com/gitpod-io/gitpod/blob/main/.werft/platform-delete-preview-environments-cron.yaml#L64

@mustard-mh
Copy link
Contributor Author

I'll rebase it to origin/main to keep it fresh

@mustard-mh mustard-mh force-pushed the hw/fix-dashboard-onboarding-bug branch from b9e3e67 to 7453b0d Compare May 6, 2022 09:17
@mustard-mh
Copy link
Contributor Author

mustard-mh commented May 6, 2022

Werft job done 🎉 cc @laushinka

@laushinka
Copy link
Contributor

Tested according to the steps. In step 5 I don't see a modal - is this the expectation?

@andreafalzetti
Copy link
Contributor

Tested according to the steps. In step 5 I don't see a modal - is this the expectation?

Step 5 -> The modal should appear again, once you've deleted all workspaces. The concept is, if you've started a workspace, the modal should not be visible.

@laushinka
Copy link
Contributor

Step 5 -> The modal should appear again, once you've deleted all workspaces.

@andreafalzetti I don't have any workspaces anymore and I don't see the modal reappear.

Screenshot 2022-05-06 at 13 17 59

@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

The modal should appear again, once you've deleted all workspaces.

@andreafalzetti I could see the modal but is this intended or planned?

From UX perspective:

Sorry for hijacking this code review! 😇

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 6, 2022

Step 5 -> The modal should appear again, once you've deleted all workspaces.

@andreafalzetti I don't have any workspaces anymore and I don't see the modal reappear.

Screenshot 2022-05-06 at 13 17 59

You have a defaultIde in your user preferences; (I've just checked in the DB), this means that you should not see the modal.

The defaultIde is also saved if you close the modal via the "X" -> in this case it defaults to code

EDIT: I've updated the PR desc "how to test" to make it clear what the expectations are

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 6, 2022

  • If a user has seen or interacted with the modal once

@gtsiolis that'a the behaviour that this PR implements. Please check again the how-to-test section, it should be clearer 🙏

@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

@andreafalzetti

Ah, so this PR catches the edge case where a new user opens a repository via a gitpod.io/# URL but never visits the dashboard, the workspace gets garbaged collected, and when they visit the dashboard for the first time they see the modal, right?

However, this will no longer be necessary once we merge #9663, correct? I could be missing something but wondering why we are not merging #9663 instead. Maybe #9663 is far from being merged or the check we're adding in this PR needs to be in there anyway? 🤷

@laushinka
Copy link
Contributor

laushinka commented May 6, 2022

You have a defaultIde in your user preferences; (I've just checked in the DB), this means that you should not see the modal.

Ah, I see. Would that defaultIde have to be removed from the DB for me to be able to test it again? 🙈

Update: I deleted my defaultIde from the DB, retook the steps and confirmed that I see the modal again.

@andreafalzetti
Copy link
Contributor

andreafalzetti commented May 9, 2022

However, this will no longer be necessary once we merge #9663, correct? I could be missing something but wondering why we are not merging #9663 instead. Maybe #9663 is far from being merged or the check we're adding in this PR needs to be in there anyway? 🤷

You have a good point @gtsiolis - For the long term, I cannot think of a current use-case where merging this - considering we'll soon merge #9663 - we add much value 🤔

In the shorter term, it could help prevent seeing the modal after you've already used a workspace once, which might have looked odd.

@mustard-mh what's your view on this? Shall we continue with this PR or abandon it?

@mustard-mh
Copy link
Contributor Author

Initially, this PR was created as a quick fix for this bug, as it was simple to modify and would be much sooner than the PR #9663 for review.

In reality, both PRs have been in the review state for a long time.

if #9663 can be merged first, we can close this one.

Edge case (How to test section step 3) will cause this issue too, but it's really edge case, we can ignore it

cc @andreafalzetti @gtsiolis

@mustard-mh mustard-mh closed this May 9, 2022
@mustard-mh mustard-mh deleted the hw/fix-dashboard-onboarding-bug branch May 11, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants