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

Refactor getStaticPageProps workflowID to project's single active workflow, if no query param #2912

Merged
merged 7 commits into from
Mar 2, 2022

Conversation

mcbouslog
Copy link
Contributor

@mcbouslog mcbouslog commented Mar 1, 2022

Package:

  • app-project

Closes #2910.

Describe your changes:

  • refactor Project store view defaultWorkflow (which was not necessarily the default workflow as defined in project.configuration.default_workflow) to singleActiveWorkflow (which is what the returned value is based on)
  • refactor ProjectHeaderContainer and all related tests per ^
  • remove default from fetchWorkflowsHelper
  • refactor getStaticPageProps to set workflowID per singleActiveWorkflow, if no query param

Note - removing references to default workflow in accordance with ADR 34

Helpful explanations that will make your reviewer happy:

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

@mcbouslog mcbouslog requested a review from eatyourgreens March 1, 2022 23:25
@mcbouslog mcbouslog added the bug Something isn't working label Mar 1, 2022
Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Code read looks good. I'll run through the code locally later today but the changes seem harmless. I've suggested clarifying the comments and also using mock data that reflects real API data.

ADR 34 says the following about default workflows, which won't be possible if they're removed completely.

Default workflows programmatically no longer has any meaning. We could still use this to visually indicate a preferred or suggested workflow from the project team in the UI. This is a design question to explore if we are asked for it.

Maybe restrict this PR just to fixing the bug and open a second PR to suggest removing default workflows completely? I've got no strong opinions.

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

@github-actions github-actions bot added the approved This PR is approved for merging label Mar 2, 2022
@mcbouslog mcbouslog merged commit 2118558 into master Mar 2, 2022
@mcbouslog mcbouslog deleted the fix-2910 branch March 2, 2022 17:04
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 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workflow selection modal showing on PH TESS
2 participants