-
Notifications
You must be signed in to change notification settings - Fork 3
org dashboard contract refactor #2837
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
base: main
Are you sure you want to change the base?
Conversation
f226648 to
9a2411b
Compare
frontends/main/src/app/dashboard/organization/[orgSlug]/contract/[contractSlug]/page.tsx
Show resolved
Hide resolved
jonkafton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Another thing we probably want to do is change some contract slugs. Currently it would be, e.g., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working well for me except the empty contract issue.
Suggestion: In the interest of caution, do you think we should leave /dashboard/organization/[orgSlug] route in place for now, rendering something like
<ContractContent // <-- modify to use first contract if contractSlug missing
orgSlug={resolved.orgSlug}
contractSlug={null}
/>Edit: Alternatively, could add a nextjs redirect for organization/[slug] to /organization, which would then redirect to most recent org contract.
| if (!contract?.programs || contract.programs.length === 0) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking Question: What's the point of this filter? I currently have an org with 3 contracts, one of which is empty. The empty contract shows all org content (but in a kinda buggy way).
Separately: I think the contract filtering should ideally be done at the request level, though probably the APIs don't support that filter yet.
| <Typography variant="subtitle2" component="span"> | ||
| {org.name} | ||
| </Typography> | ||
| {` - ${contract?.name}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github suggestions seem to have disappeared for me...
But contract.name would be fine, it's not nullable.
| const contract = b2bOrganization.contracts[0] | ||
| if (contract) { | ||
| const contractUrl = contractView( | ||
| b2bOrganization.slug.replace("org-", ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove all the org prefixes in prod, IMO, and drop all the org- stripping shenanigans.
Since wagtail slugs only need to be unique within their parent, and all organization pages are children of the organization index page, it should be fine to drop the prefix. And client side we've always stripped the prefix, so there should be no behavior change for users.
Separate issue, though.
| invariant(resolved.orgSlug, "orgSlug is required") | ||
| invariant(resolved.contractSlug, "contractSlug is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have similar invariant calls in place elsewhere. Before NextJS added that PageProps thing, it wouldn't have known that those params were non-nullale.
In the last commit (a5149ad) I added this functionality you suggested. It either displays the first available contract or redirects to the dashboard home if none are found. What do you think? |
…a single contract as context, then iterate all contracts on the dashboard nav
… for that org or back to the dashboard home if no contracts are found
a5149ad to
e3d8e54
Compare

What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9679
Description (What does it do?)
This PR reworks functionality in the dashboard surrounding b2b organizations / contracts:
/dashboard/organization/org-slug/contract/contract-slugOrganizationContentcomponent was refactored / renamed toContractContentand now requires a contract ID along with the org ID.Screenshots (if appropriate):
How can this be tested?
/dashboard/organization/org-slug/contract/contract-slug)Checklist: