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.

Sign up for GitHub

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

Jump to bottom

fix filename & title getting improperly defaulted in filesUploadV2 #1346

Merged
merged 6 commits into from
Aug 19, 2024
Merged

fix filename & title getting improperly defaulted in filesUploadV2 #1346

merged 6 commits into from
Aug 19, 2024

Conversation

Cheos137
Copy link
Contributor

@Cheos137 Cheos137 commented Aug 13, 2024 •
edited
Loading

fix filename & title getting improperly defaulted when uploading multiple files using MethodsClientImpl#filesUploadV2

fixes #1345

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.

@Cheos137
fix filename & title getting improperly defaulted when uploading mult…
be90ffa 
…iple files using MethodsClientImpl#filesUploadV2
@salesforce-cla Salesforce CLA
Copy link

Thanks for the contribution! Before we can merge this, we need @Cheos137 to sign the Salesforce Inc. Contributor License Agreement.

@Cheos137 Cheos137 mentioned this pull request Aug 13, 2024
Closed
@zimeg zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client labels Aug 15, 2024
@zimeg zimeg added this to the 1.40.4 milestone Aug 15, 2024
@zimeg
Copy link
Member

zimeg commented Aug 15, 2024

Hi @Cheos137! 👋 Thank you for taking the time to send in this PR and sign the CLA!

This logic is looking solid to me but we might want to include a test to ensure this behavior is correct and doesn't regress in future changes 🙏

@codecov Codecov
Copy link

codecov bot commented Aug 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.99%. Comparing base (71ed00d) to head (a50d102).
Report is 3 commits behind head on main.

Files Patch % Lines
.../com/slack/api/methods/impl/MethodsClientImpl.java 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #1346   +/-   ##
=========================================
  Coverage     74.98%   74.99%           
  Complexity     4189     4189           
=========================================
  Files           451      451           
  Lines         12930    12929    -1     
  Branches       1331     1331           
=========================================
  Hits           9696     9696           
+ Misses         2463     2462    -1     
  Partials        771      771

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Cheos137
implement unit tests accompanying slackapi#1346
3391a58 
these tests check the handling and defaulting of filename and title when using filesUploadV2, specifically when uploading multiple files / using JUST the uploadFiles parameter
@Cheos137
Copy link
Contributor Author

cuz i suspect there might not have gone out a notification from it: yesterday i've added tests to this pr

zimeg
zimeg reviewed Aug 16, 2024
View reviewed changes
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

Thanks a ton for adding the tests! Once these are passing I think the PR will be in a solid spot 🙏

slack-api-client/src/test/java/test_with_remote_apis/methods/files_Test.java Outdated Show resolved Hide resolved
@Cheos137 Cheos137 requested a review from zimeg August 17, 2024 08:40
zimeg
zimeg reviewed Aug 18, 2024
View reviewed changes
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

This is looking solid! I did leave one question about the defaults being tested that I'm unsure of myself- let me know if setting specific strings seems right 🧵 👀

slack-api-client/src/test/java/test_with_remote_apis/methods/files_Test.java Outdated Show resolved Hide resolved
slack-api-client/src/test/java/test_with_remote_apis/methods/files_Test.java Outdated Show resolved Hide resolved
@Cheos137 @zimeg
Fix using a nonexistent variable in a test
a757417 
i seem to have been really sleepy when writing them

Co-authored-by: Eden Zimbelman <[email protected]>
@Cheos137 Cheos137 requested a review from zimeg August 18, 2024 07:33
zimeg
zimeg reviewed Aug 18, 2024
View reviewed changes
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

📝 🎶 A few more notes, but thank you for the fast fixes already!

slack-api-client/src/test/java/test_with_remote_apis/methods/files_Test.java Outdated Show resolved Hide resolved
slack-api-client/src/test/java/test_with_remote_apis/methods/files_Test.java Outdated Show resolved Hide resolved
@Cheos137 Cheos137 requested a review from zimeg August 18, 2024 09:33
@seratch seratch self-assigned this Aug 19, 2024
seratch
seratch approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thank you so much for fixing this! Looks great to me 👍

@seratch seratch merged commit d4622ca into slackapi:main Aug 19, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

@zimeg zimeg Awaiting requested review from zimeg

Assignees

@seratch seratch

Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented cla:signed project:slack-api-client project:slack-api-client
Projects
None yet
Milestone
1.41.0
Development

Successfully merging this pull request may close these issues.

Uploading multiple files overrides filename
3 participants
@Cheos137 @zimeg @seratch