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

Multi-tenant navigation with PA placeholder content #393

Merged
merged 6 commits into from
Apr 15, 2021

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented Apr 14, 2021

Description of the change

Completes the initial preparations to support multiple tenants. In the final analysis this wound up including:

  • adds placeholder content for PA. This is the bare minimum required to let you navigate around to the expected pages, but there is neither copy nor data yet for the metrics so I just left them out for now.
  • replaces the automatic redirect to ND with a check for a "locked" Tenant in the data store. If the lock is engaged, the homepage will redirect to that tenant's home page, and direct visits to any other tenants' paths will result in a Not Found page.
  • Adds a basic homepage with links to the available tenants. This is only intended for use in local and staging environments, though it will technically also be in prod if you manage to visit the base Firebase URL.
  • Adds a feature flag for granular tenant releases per environment. This will make intermediate prod releases safer because we can suppress the PA pages entirely even while they are available in staging. (Direct visits to the url of a disabled tenant also result in Page Not Found, so it should look the same to an end user as if the tenant didn't exist.)

I tested as much of this functionality as I could in Jest but supplemented it with some pretty thorough manual testing, by editing my hosts file to point the domains of interest at my local dev server. There should be no change in behavior for the existing ND prod domain, and the new staging domains also worked as expected. (I also tested this in staging a bit but that environment isn't necessarily up to date with this branch now.)

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #389, closes #391

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@coveralls
Copy link

coveralls commented Apr 14, 2021

Pull Request Test Coverage Report for Build 753224932

  • 30 of 32 (93.75%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-12.7%) to 60.466%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotlight-client/src/DataStore/TenantStore.ts 5 6 83.33%
spotlight-client/src/PageHome/PageHome.tsx 6 7 85.71%
Totals Coverage Status
Change from base Build 745401377: -12.7%
Covered Lines: 1125
Relevant Lines: 1829

💛 - Coveralls

Copy link

@jovergaag jovergaag left a comment

Choose a reason for hiding this comment

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

I wasn't able to test it locally, but the testing looks thorough! I realized in reading this that I'm confused about the URL that users are using to access the app. Pls help :)

reactImmediately(() => {
expect(tenantStore.currentTenant?.id).toBe("US_ND");
expect(tenantStore.currentTenantId).toBe("US_ND");
expect(tenantStore.locked).toBe(true);

Choose a reason for hiding this comment

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

There isn't a case where an org like CSG can view multiple tenants? I guess in this case they would not have a ".gov" email address... Just wondering if there is ever a use case that we would want to allow multiple tenant users.

Choose a reason for hiding this comment

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

Actually, as I have read this more thoroughly it looks like the tenant is coming from the current location. I guess I'm unclear how this is deployed in prod and the DNS magic that is allowing us to put this behind a .nd.gov domain? Is this part of the agreement with ND?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it's a bit convoluted. We do not, as yet, actually have a spotlight.recidiviz.org domain configured for prod. For ND they did indeed configure their own DNS to point dashboard.docr.nd.gov at our prod Firebase environment, which has been fine because the whole site just defaults to ND as the only tenant. But now the expectation is that PA is going to do the same with something.pa.gov, and eventually if we get a few more states on the platform we are going to want spotlight.recidiviz.org to exist as well.

And then the additional staging subdomains just let us replicate those different experiences in the pre-release environment. (The default staging domain being spotlight-staging.recidiviz.org, which is where you can access all the tenants at once)

Does that make sense?

Choose a reason for hiding this comment

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

Yes it does, very helpful thanks!

expect(scrollSpy).toHaveBeenNthCalledWith(1, 0, 0);
expect(scrollSpy).toHaveBeenNthCalledWith(2, 0, 0);
});
expect(scrollSpy).toHaveBeenCalledWith(0, 0);

Choose a reason for hiding this comment

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

Cool GibHub Actions linting error integration above!

@macfarlandian macfarlandian merged commit c7e4634 into main Apr 15, 2021
@macfarlandian macfarlandian deleted the ian/389-add-pa branch April 15, 2021 22:44
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.

Multi-tenant homepage add placeholder content for PA
3 participants