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

Overhaul the Domain server's onboarding wizard. #1344

Merged
merged 23 commits into from
Oct 4, 2021

Conversation

digisomni
Copy link
Member

@digisomni digisomni commented Sep 13, 2021

QA: Please go ahead and download this and give feedback about the general flow of things and if there are any issues.

  • Go to your PR server's config.json and set completed_once to false to force the tutorial to show up again.

@digisomni digisomni added enhancement New feature or request domain-server Issues related to the domain server in general needs testing (QA) The PR is ready for testing labels Sep 13, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Sep 13, 2021
@ctrlaltdavid
Copy link
Collaborator

Did you intend to include the files in the /dist (i.e., build?) directory in the PR?

@Misterblue
Copy link
Contributor

Did the setup and when I got to the "Create a username and password for admin panel" I could not see the text areas where things should be input. Attached is a screen cap and you can see the some text area refresh/control icons but the text input area is not visibl
AdminLoginSetup-20210919a
e.

@Misterblue
Copy link
Contributor

On setting up an admin account, I made some mistake because I couldn't log into the browser again. Looking at AppData/Roaming/Vircadia - PR1344/domain-server/config.json, I discovered that I had input "Misterblue" and trying to login with "misterblue" failed. Logging in the "Misterblue" worked.
Account names should be case insensitive.

@digisomni
Copy link
Member Author

Did the setup and when I got to the "Create a username and password for admin panel" I could not see the text areas where things should be input. Attached is a screen cap and you can see the some text area refresh/control icons but the text input area is not visibl
AdminLoginSetup-20210919a
e.

What browser and version of the browser are you using? Also I notice that you may be using the first build instead of the latest updated build. Though it should work in either case.

Do you have your OS set to use dark or light theme?

@Misterblue
Copy link
Contributor

It was the latest chrome. I believe I'm set on light mode. But the other dialogues worked fine, it was just the admin login dialogue that was the wrong colors

@ctrlaltdavid
Copy link
Collaborator

Did you intend to include the files in the /dist (i.e., build?) directory in the PR?

Yes.

It seems weird to commit files in the /dist (i.e., build) directory.
Also, what is the source of the /dist/spa files? They appear to be "binaries" (i.e., built files) but where they came from (one of the plug-ins, some other codebase, ...) isn't clear.

@digisomni
Copy link
Member Author

You're right, it's the quick and easy way to do it. The alternative is to have it trigger like the server-console as a CMake step because it would be necessary for it to be built at compile time using npm then Quasar. I think that's something that can and probably should be setup at some point: a resources bootstrap step that we can toss this and any future NPM commands into.

dist/spa is where the built files go, SPA stands for single page app which is what this is.

@ctrlaltdavid
Copy link
Collaborator

Some build instructions should be added on how to build and commit updates.

@digisomni
Copy link
Member Author

Are there any sorts of specific instructions needed? So far there is a README.md explaining how to work with the project.

@ctrlaltdavid
Copy link
Collaborator

In the "Build the app" section perhaps something along the lines of...

  • "This builds the wizard app in the /dist/spa directory."
  • How these files are used (do you have to manually copy them somewhere in order for the domain server to use them or is it all automatic.
  • Instructions to check them into source control when make an update to the code.

Copy link
Collaborator

@ctrlaltdavid ctrlaltdavid left a comment

Choose a reason for hiding this comment

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

Looks OK though I haven't properly reviewed the Vue-related items because I don't know Vue.

Copy link
Contributor

@Misterblue Misterblue left a comment

Choose a reason for hiding this comment

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

I don't see anything wrong with the Vue code (based on my limited knowledge of same).
Some misc comments and questions were added.

import axios, { AxiosInstance } from "axios";
import Log from "../modules/utilities/log";

declare module "@vue/runtime-core" {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a comment that is declarization adds ".$axios" reference to all Vue components.

And, is this in addition to the addition of ".$axios" to app.config.globalProperties done below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I couldn't tell you exactly how this works as it's boilerplate since I bootstrapped the project with Quasar CLI to include Axios.

domain-server/resources/web/web-new/src/boot/axios.ts Outdated Show resolved Hide resolved
@@ -86,7 +86,16 @@ bool DomainServer::_getTempName { false };
QString DomainServer::_userConfigFilename;
int DomainServer::_parentPID { -1 };

/// @brief Route a request to the Metaverse server.
/// @param connection The HTTP connection object.
/// @param requestUrl The full URL of the request.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should mention the optional 'key' query. This doesn't say much about what a "full URL" could possibly be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see anything regarding an optional key query in the parameters for this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might have misunderstood the added code. It looks like code was added to check the query parameters on the requestUrl for the presence of different fields. This is not made clear in the description or explanation of the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated it. Also the added code replaces the old code. The old code was not working correctly, so I did it a different way while fixing it.

Copy link
Contributor

@Misterblue Misterblue left a comment

Choose a reason for hiding this comment

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

Worked great.

@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing needs CR (code review) labels Oct 4, 2021
@digisomni digisomni merged commit 1d8da47 into vircadia:master Oct 4, 2021
@digisomni digisomni deleted the feature/new-domain-wizard branch October 4, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Approved At least one code reviewer has approved the PR. domain-server Issues related to the domain server in general enhancement New feature or request QA Approved The PR has been tested successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants