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

Project: Add 404 responses for projects and workflows #1964

Merged
merged 10 commits into from
Feb 15, 2021
Merged

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Jan 8, 2021

Check the project model for a project ID after the project API call resolves. If there's no project ID, set a 404 status code and show the default Next error page.

When there's a workflow ID in the URL, return 404 if it isn't listed in the project's active workflows.

Split data-fetching, and page props that only depend on the page URL, into a getStaticPageProps helper.

Screenshot of a blank white error page, saying: 404, project btooke/i-fancy-cats was not found.

Package:
app-project

Closes #1694.

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?

Components

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

  • Is the changelog updated?
  • Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

@eatyourgreens eatyourgreens requested a review from a team January 8, 2021 15:38
@eatyourgreens eatyourgreens added the enhancement New feature or request label Jan 8, 2021
@eatyourgreens eatyourgreens changed the title Project: Return 404 if the project load fails Project: Add 404 responses for projects and workflows Jan 25, 2021
@eatyourgreens
Copy link
Contributor Author

I think this PR will 404 if Panoptes is down. Do we want to do that? The project model might need a bit of work to distinguish between API errors and successful requests for resources that don't exist.

@eatyourgreens eatyourgreens force-pushed the project-404 branch 2 times, most recently from bfc4c37 to 4a2dc85 Compare January 25, 2021 13:16
@eatyourgreens
Copy link
Contributor Author

The size of this PR jumped a lot today, but the increase is mostly tests. A lot of the SSR page props code has never had tests.

@eatyourgreens eatyourgreens force-pushed the project-404 branch 3 times, most recently from 559ca9a to 1bd5732 Compare January 26, 2021 12:45
@srallen srallen self-assigned this Feb 12, 2021
Copy link
Contributor

@srallen srallen left a comment

Choose a reason for hiding this comment

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

One minor style thing on the current generic 404 page:

Screen Shot 2021-02-12 at 2 28 52 PM

We have an extra period in the message. This is a non-issue since I think implementing @beckyrother's designs would be the next iteration?

Nice set of new tests for this functionality with nock! Also, I since this partially implements what we discussed in #1997, it'd be nice to have the summary ADR of the decision written up to be able to reference while reviewing. Any chance you have time to write that up? Thanks!

packages/app-project/stores/Project.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved This PR is approved for merging label Feb 12, 2021
Check the project model for a project ID after the project API call resolves. If there's no project ID, set a 404 status code and show the default Next error page.
Send back 404 if there's a workflow ID in the URL and it isn't an active workflow.
Perform all data fetching in getStaticPageProps (props that only depend on the page URL.) Return entry with a 404 to avoid extra API calls when we know there's nothing to fetch.
Add a couple of empty responses to the mocked API for getDefaultProps. Wrap the existing tests in describe blocks.
Essentially the same tests as for default page props, but with the HTTP request and response removed.
Rename NotFoundError to notFoundError and add tests.
Remove a redundant existence check. Remove full stops from error messages (it seems the Next error page adds these in.)
@eatyourgreens eatyourgreens merged commit cbdada7 into master Feb 15, 2021
@eatyourgreens eatyourgreens deleted the project-404 branch February 15, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'active_workflows' of undefined
2 participants