UserManager: show proper org domain (#476) #1143
Conversation
jordigh
force-pushed
the
jordigh/fix-org-display
branch
from
c3c1217 to
bda85cf
Compare
jordigh
changed the title
jordigh
changed the title
jordigh
changed the title
| return [ | ||
| t('Manage members of team site'), | ||
| !resource ? null : cssOrgName( | ||
| `${(resource as Organization).name} (`, | ||
| cssOrgDomain(`${(resource as Organization).domain}.getgrist.com`), |
There was a problem hiding this comment.
Previously we used .domain prop, and now you calculate it using .baseDomain config.
Can you explain why? And also I see this comment on baseDomain:
// Base domain for constructing new URLs, should start with "." and not include port, e.g.
// ".getgrist.com". It should be unset for localhost operation and in single-org mode.This should be unset looks suspicious. Are you sure it will work?
There was a problem hiding this comment.
domain is the subdomain, baseDomain is what was hardcoded as .getgrist.com.
You were right, though, I should be using domain for the orgDisplay not name.
app/client/ui/UserManager.ts
Outdated
| const gristConfig = getGristConfig(); | ||
| const gristHomeHost = gristConfig.homeUrl ? new URL(gristConfig.homeUrl).host : ''; | ||
| const baseDomain = gristConfig.baseDomain || gristHomeHost; | ||
| const orgName = (resource as Organization).name; |
There was a problem hiding this comment.
Later in the code you have resource ? ... : ... code fragment, which checks if resource is null or not, but here, you ignore this check and just grab the .name prop. Can you refactor it?
There was a problem hiding this comment.
How about this refactoring?
We had `getgrist.com` hardcoded here, which only works for SaaS. The base domain as well as the way that orgs are encoded in the URL can be different in other circumstances. If we are encoding orgs in the domain name, that's easy. We just do `orgname.base.domain.name`. If we are not, then we first try a base domain, and if that isn't set, we'll use the domain of the home server.
jordigh
force-pushed
the
jordigh/fix-org-display
branch
2 times, most recently
from
b0e9174 to
e4f85bc
Compare
| const gristConfig = getGristConfig(); | ||
| const gristHomeHost = gristConfig.homeUrl ? new URL(gristConfig.homeUrl).host : ''; | ||
| const baseDomain = gristConfig.baseDomain || gristHomeHost; | ||
| const orgDisplay = isOrgInPathOnly() ? `${baseDomain}/o/${org.domain}` : `${org.domain}${baseDomain}`; |
There was a problem hiding this comment.
I don't like the possibility that a baseDomain can be empty, and then gristHomeHost will be something like foo.example.com and then you concatenate it with a domain name which will produce something like catsfoo.example.com without a dot in the middle.
I tried reproducing this with various setups but couldn't - baseDomain was always set, so I think it is fine.
We had
getgrist.comhardcoded here, which only works for SaaS. The base domain as well as the way that orgs are encoded in the URL can be different in other circumstances.If we are encoding orgs in the domain name, that's easy. We just do
orgname.base.domain.name. If we are not, then we first try a base domain, and if that isn't set, we'll use the domain of the home server.fixes: #476