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

Add external_id to WorkflowStepView #1392

Merged
merged 3 commits into from Dec 3, 2021

Conversation

srajiang
Copy link
Member

@srajiang srajiang commented Dec 3, 2021

Summary

Fixes #1391 by adding external_id properties required by views.update method.

Requirements (place an x in each [ ])

@srajiang srajiang added enhancement M-T: A feature request for new functionality area:typescript issues that specifically impact using the package from typescript projects labels Dec 3, 2021
@srajiang srajiang added this to the types@2.4 milestone Dec 3, 2021
@srajiang srajiang self-assigned this Dec 3, 2021
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Hmm, now that I'm reviewing this, I'm wondering if these changes are correct. @seratch @srajiang can you double-check my assumptions below?

In digging into where these types are used, I can see that it is consumed in the @slack/web-api package: the super-type View is imported in src/methods.ts, a e.g. ViewsUpdateArguments interface uses the View as a view property and finally the e.g. views.update method uses ViewsUpdateArguments as the arguments to be passed in.

However, the optional external_id and view_id properties are set as properties on this ViewsUpdateArguments interface inside @slack/web-api - not on the WorkflowStepView type exported by @slack/types.

With that context, perhaps this isn't the right change? It seems like the various *View types exported by @slack/types are meant to model the view object to be passed into the various views.* API methods.

Perhaps there is nothing wrong here? Am I totally off-base?

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Nevermind! @srajiang and @bkeung astutely pointed out that the View object itself can also have the IDs as part of the responses coming back. My mistake!

@srajiang
Copy link
Member Author

srajiang commented Dec 3, 2021

I have removed view_id from the type for now as Fil's pointed out view_id is set at the ViewsUpdateArguments and is not listed in the general views documentation.

@srajiang srajiang changed the title Add external_id and view_id to WorkflowStepView Add external_id to WorkflowStepView Dec 3, 2021
@srajiang srajiang merged commit 62147ad into slackapi:main Dec 3, 2021
@srajiang srajiang deleted the update-workflow-step-view branch December 3, 2021 19:28
@seratch
Copy link
Member

seratch commented Dec 3, 2021

Thanks for working on this! Not having view_id this time is fine, but we updated the view object in API response, not the API argument list here. As long as the actual payload has view_id, there is no reason not to have it. We may improve the incomplete views document page plus add the property too in the future.

srajiang added a commit that referenced this pull request Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects enhancement M-T: A feature request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WorkflowStepView doesn't work if there are multiple views.update API calls
3 participants