-
Notifications
You must be signed in to change notification settings - Fork 31
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
EN-376 Add contributors and note to language variants #279
base: master
Are you sure you want to change the base?
Conversation
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.
PR mentions only due date being added to Change workflow endpoint, but it appears much more's been added in the end. nothing wrong with that, just wanted to clarify the extent the endpoint is being modified and if it's retaining its name in API reference (otherwise the underlying method should likely be renamed too). suggested a minor formatting change, otherwise looks good
/// <param name="workflowStepIdentifier">Workflow step identifier to set.</param> | ||
Task ChangeLanguageVariantWorkflowAsync(LanguageVariantIdentifier identifier, WorkflowStepIdentifier workflowStepIdentifier); | ||
/// <param name="changeModel">Change language variant workflow model.</param> | ||
Task ChangeLanguageVariantWorkflowAsync(LanguageVariantIdentifier identifier, ChangeLanguageVariantWorkflowModel changeModel); |
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.
will this endpoint still be called "Change the workflow of a language variant" in the API reference? if not, shouldn't the method be renamed?
@@ -2025,7 +2025,15 @@ public async void PutVariantWorkflow() | |||
var exception = await Record.ExceptionAsync(async () => | |||
await client.ChangeLanguageVariantWorkflowAsync( | |||
new LanguageVariantIdentifier(itemIdentifier, languageIdentifier), | |||
new WorkflowStepIdentifier(Reference.ById(Guid.Empty), workflowStepIdentifier) | |||
new ChangeLanguageVariantWorkflowModel(Reference.ById(Guid.Empty), workflowStepIdentifier) |
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.
Since this file is apparently used for code samples, I'd probably fix the formatting here.
Motivation
The goal is to support contributors and notes in language variants. This includes the enhancement of "change-workflow" endpoint that where the due date was also added.
Checklist
How to test
If manual testing is required, what are the steps?