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

feat: add initial workflow status in config #6469

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

RomainOliva
Copy link

@RomainOliva RomainOliva commented May 11, 2022

Summary

closes #1306

For our case we now use the editional workflow, we use it to preview our changes, but sometimes we just want to publish directly.
And this is not possible, we have to save, change status to ready and publish.

I would have preferred to do this issue #4054 but it seemed more complex for a first issue on the project :)

The goal here is to add the initial status in the configuration, then in our cas we can put PENDING_PUBLISH status by default and then publish directly.

Test plan

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • Code is formatted via running yarn format.
  • Tests are passing via running yarn test.
  • The status checks are successful (continuous integration). Those can be seen below.

Cute animal

@RomainOliva RomainOliva force-pushed the feat/add-default-workflow-status-config branch 3 times, most recently from 1be43b9 to 049c87a Compare May 11, 2022 08:38
@RomainOliva RomainOliva marked this pull request as ready for review May 11, 2022 08:38
@RomainOliva RomainOliva requested a review from a team May 11, 2022 08:38
@RomainOliva
Copy link
Author

I don't really know where to add the expected test, if you can guide me :)

@bytrangle bytrangle added kind: feature type: feature code contributing to the implementation of a feature and/or user facing functionality labels May 30, 2022
@bytrangle bytrangle self-assigned this May 30, 2022
@@ -29,7 +29,7 @@ Example: The exact commands you ran and their output, screenshots / videos if th

Please add a `x` inside each checkbox:

- [ ] I have read the [contribution guidelines](../CONTRIBUTING.md).
- [ ] I have read the [contribution guidelines](https://github.com/netlify/netlify-cms/blob/master/CONTRIBUTING.md).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have gone to a different PR but I'm going to accept it since its scope is small.

Copy link
Collaborator

@bytrangle bytrangle left a comment

Choose a reason for hiding this comment

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

Very neat editing.

As for testing, I haven't tried it out but it might be about checking if the unpublished entry object has the default_workflow_status property with the value that a user sets in the config.

I think it could go something like this:

  • create a placeholder config object which contains the default_workflow_status property
  • create a new backend instance, each for all the supported backends.
  • create a state object which contains the config object: state = { config, ... }
  • call the unpublishedEntry method of the Backend class, passing to it the state object.
  • check if the result contains a default_workflow_status property whose value is whatever set in the config object

What do you think?

@RomainOliva
Copy link
Author

Hello @bytrangle, thanks for your review.

I tried when I start the backend, and everything work right.
But inside test I can't find where I can test this behavior, in the file that you have pointed out to me it doesn't work because it's looks like it's not in "editorial_workflow".

I tried here :

And here :

Can you give me a little help please?

@bytrangle
Copy link
Collaborator

bytrangle commented Jun 2, 2022

On further investigation, I think the test should be in netlify-cms-core/.../actions/__tests__/config.spec.js.

I got the idea from the test "should set publish_mode from config". I've tried create a barebone config object with two props: publish_mode and default_workflow_status.

Then I called applyDefaults(config). This function takes the original config object parsed from config.yml file and mutate it so that certain important properties won't be undefined if the user forgot to add them.

For example, publish_mode has to be SIMPLE_WOKFLOW, slug has to be an empty object, collections is an empty array unless specified otherwise in the config.

Then I check if applyDefaults(config).default_workflow_status is equal to the value I passed to the namesake property of the config.

You'll also need to modify the applyDefaults(config) so that if the user sets publish_mode to editorial_workflow, and don't specify the default_workflow_status, the mutated config will have a property default_workflow_status set to draft.

Does that make sense?

@RomainOliva
Copy link
Author

RomainOliva commented Jun 2, 2022

@bytrangle Oh nice ! It's really clear, I will do that :)

Just for your last sentence, you really think it's useful with the following code ?

With this line if there is no default_workflow_status it take the first status (draft one)

thanks again for your help

@RomainOliva
Copy link
Author

@bytrangle I just add a fixup commit with some change and the test, tell me if this sounds good to you :)

@RomainOliva RomainOliva force-pushed the feat/add-default-workflow-status-config branch from 4798191 to 78cc7e0 Compare June 2, 2022 15:56
bytrangle
bytrangle previously approved these changes Jun 6, 2022
@bytrangle
Copy link
Collaborator

@krider2010 Look forward to your review.

@krider2010
Copy link

Just a heads up I'm working on being added to the maintainer group for review reasons.

@RomainOliva
Copy link
Author

@bytrangle There is no other reviewer who can watch this pr? :)

@RomainOliva
Copy link
Author

RomainOliva commented Jun 23, 2022

@bytrangle Small up please, this feature would allow us to save a lot of time in our processes 🙈

@stale
Copy link

stale bot commented Apr 26, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Apr 26, 2023
@RomainOliva
Copy link
Author

It is still relevant today 🙈

@stale stale bot removed the status: stale label Apr 26, 2023
@emp3ror
Copy link

emp3ror commented May 15, 2023

It is still relevant today see_no_evil

waiting for this to happen

@martinjagodic
Copy link
Member

@RomainOliva are you still interested in moving this forward?

@RomainOliva
Copy link
Author

@martinjagodic Yes, it can be interesting to merge it 😇

@martinjagodic
Copy link
Member

Great! Can you solve the merge conflicts? They mostly emerged because of the package rename.

@RomainOliva RomainOliva force-pushed the feat/add-default-workflow-status-config branch from 08255b6 to cb2ee3d Compare November 10, 2023 15:56
Copy link

netlify bot commented Nov 10, 2023

‼️ Deploy request for cms-demo rejected.

Name Link
🔨 Latest commit cb2ee3d

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for decap-www ready!

Name Link
🔨 Latest commit cb2ee3d
🔍 Latest deploy log https://app.netlify.com/sites/decap-www/deploys/654e52adad3a9d00086355ce
😎 Deploy Preview https://deploy-preview-6469--decap-www.netlify.app/docs/configuration-options
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@RomainOliva
Copy link
Author

@martinjagodic sorry for the delay, it's all good the PR is rebased :)

@RomainOliva RomainOliva requested a review from a team as a code owner March 29, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature pinned type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants