Skip to content

feat: Add "detail" parameter to CanvasesCreateResponse & CanvasesEditResponse#1570

Open
Lexer11l wants to merge 8 commits intoslackapi:mainfrom
Lexer11l:add_title_canvas_response
Open

feat: Add "detail" parameter to CanvasesCreateResponse & CanvasesEditResponse#1570
Lexer11l wants to merge 8 commits intoslackapi:mainfrom
Lexer11l:add_title_canvas_response

Conversation

@Lexer11l
Copy link
Contributor

@Lexer11l Lexer11l commented Mar 24, 2026

Adding detail to CanvasesCreateResponse & CanvasesEditResponse

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

Issue:
#1568

feat: Add "title" parameter to CanvasesCreateResponse
Added tests for error handling in canvases.create and canvases.edit methods.
@Lexer11l Lexer11l requested a review from a team as a code owner March 24, 2026 09:42
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Hi @Lexer11l thanks for opening this PR 🙏

The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

@Lexer11l
Copy link
Contributor Author

Hi @Lexer11l thanks for opening this PR 🙏

The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

Hi @WilliamBergamin, sure!
I was going to add it, but it required me to update widely used MockSlackApi, though I wanted to minimize the scope.
I'll push my changes so you could advise whether it's correct or how to update it 🙏

@Lexer11l
Copy link
Contributor Author

Hi @Lexer11l thanks for opening this PR 🙏
The changes seem accurate, would you also be able to update slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java to include your changes

Hi @WilliamBergamin, sure! I was going to add it, but it required me to update widely used MockSlackApi, though I wanted to minimize the scope. I'll push my changes so you could advise whether it's correct or how to update it 🙏

@WilliamBergamin I figured out that the way how mock is set up doesn't allow setting up successful and failing cases at the same time because text fixture is read only from one file:
String body = reader.readWholeAsString(methodName + ".json");
Currently, CanvasesTest tests only the successful case. If I want to add a failure test, then I'd need either to replace successful tests in this class, or introduce a workaround in MockSlackApi to override test fixture based on some condition. For example, the new token.

What would you suggest here?

@WilliamBergamin WilliamBergamin changed the title feat: Add "title" parameter to CanvasesCreateResponse & CanvasesEditResponse feat: Add "detail" parameter to CanvasesCreateResponse & CanvasesEditResponse Mar 25, 2026
@WilliamBergamin
Copy link
Contributor

What would you suggest here?

For this scenario I think its fine if we omit the changes to slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java, it seems like you changes might be missing an import once fixed we should be in a good state to approve and merge

@Lexer11l
Copy link
Contributor Author

What would you suggest here?

For this scenario I think its fine if we omit the changes to slack-api-client/src/test/java/test_locally/api/methods/CanvasesTest.java, it seems like you changes might be missing an import once fixed we should be in a good state to approve and merge

Alright, I added the missing import

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Would you be able to share with me the manifest of the app you used to run the integration tests 🤔

I'm not able to get your test to pass from my end

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running test_with_remote_apis.methods.canvases_Test
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.853 s <<< FAILURE! -- in test_with_remote_apis.methods.canvases_Test
[ERROR] test_with_remote_apis.methods.canvases_Test.error_detail -- Time elapsed: 0.571 s <<< FAILURE!
java.lang.AssertionError: 

Expected: is not null
     but: was null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants