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

Add baseDomain to user #298

Merged
merged 1 commit into from
Mar 7, 2023
Merged

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Mar 7, 2023

Fixes #253

In a few recent issues and PRs, I've noticed that we need to have access to the user's base domain (i.e., {username}.{root-domain}). For example, @Genne23v needs it in #289.

I've adjusted our code so that a User is a combination of a Prisma User (e.g., what is in the db), but also adds a dynamically generated property .baseDomain, which is the user's base domain. This baseDomain is then available in the front-end using the useUser() hook.

I have an example of how to use it by including it in the New Domain form, which we discussed in the early UI meetings:

Screenshot 2023-03-07 at 11 32 10 AM

Now a user only has to enter the first part of the name, and we build the rest for them.

I've also dealt with usernames including a ., and removed it. To make it easier to test this (until @cychu42 gets our db tests working in #238), I've updated the user hansolo to be han.solo. In the screenshot above, you can see that the . was removed from the username. I've left the username untouched in the db, though, so it matches what Seneca has (i.e., I only remove the . from the base domain we use for such a user).

Tagging a few people for reviews, since this touches a bunch of back-end and front-end code.

@humphd humphd added this to the Milestone 0.5 milestone Mar 7, 2023
@humphd humphd self-assigned this Mar 7, 2023
Copy link
Contributor

@sfrunza13 sfrunza13 left a comment

Choose a reason for hiding this comment

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

Looked it over, I can't think of any improvements, this seems good to me. I guess one thing I question is whether the . would be the only special character to worry about?

@humphd
Copy link
Contributor Author

humphd commented Mar 7, 2023

Looked it over, I can't think of any improvements, this seems good to me. I guess one thing I question is whether the . would be the only special character to worry about?

As far as I understand it, speaking to ITS, it's the only thing that might be included that we care about (e.g., they use /[a-z-_.]+/. Of these, only . is problematic.

Copy link
Contributor

@sfrunza13 sfrunza13 left a comment

Choose a reason for hiding this comment

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

fan of the aliasing and extension of user 😄

@humphd humphd mentioned this pull request Mar 7, 2023
@@ -40,7 +64,3 @@ export async function updateUserByUsername(
},
});
}

export async function deleteUserByUsername(username: User['username']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, do we not need the delete or update functionality here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We aren't the source of truth on user info, we just store it for lookup purposes. We don't really manage user info. I can't think of a case where we'd do update or delete, can you? Maybe when we add the admin UI, but currently this is dead code, and dead code should get removed.

@sfrunza13 sfrunza13 merged commit 41cbfc7 into DevelopingSpace:main Mar 7, 2023
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.

Deal with . in usernames when generating domain names
3 participants