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

Fix #1336 missing and invalid properties in rich text block elements #1337

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

seratch
Copy link
Member

@seratch seratch commented Jul 16, 2024

This pull request resolves #1336

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.

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-model project:slack-api-model labels Jul 16, 2024
@seratch seratch added this to the 1.40.3 milestone Jul 16, 2024
@seratch seratch self-assigned this Jul 16, 2024
Copy link
Member Author

@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.

comments for reviewers

private final String type = TYPE;
private String url;
private String text;
private Boolean unsafe;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also missing

private String format;
private TextStyle style;
private String url;
private String fallback;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this missing property too

public static final String TYPE = "date";
private final String type = TYPE;
private String timestamp;
private String format;
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this missing property too

@@ -42,7 +69,7 @@ public static class Channel implements RichTextElement {
public static final String TYPE = "channel";
private final String type = TYPE;
private String channelId; // C12345678
private TextStyle style;
private NoCodeTextStyle style;
Copy link
Member Author

Choose a reason for hiding this comment

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

channel, user, usergroup, team, broadcast, color do not accept code property

@@ -146,7 +150,23 @@ public static class TextStyle {
private boolean bold;
private boolean italic;
private boolean strike;
private boolean highlight;
Copy link
Member Author

Choose a reason for hiding this comment

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

these are not mentioned in the documents but actually these properties work for all elements

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 this makes sense, from what I was able to find there are currently only 2 versions of these style objects

@Builder
@NoArgsConstructor
@AllArgsConstructor
public static class NoCodeTextStyle {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this for the ones that do not accept code

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this object may be referenced as limited_style, what do you think about naming this LimitedTextStyle?

Suggested change
public static class NoCodeTextStyle {
public static class LimitedTextStyle {

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Renamed it.

@Test
public void parseRichTextElements() {
String json = "{\n" +
" \"blocks\": [\n" +
Copy link
Member Author

Choose a reason for hiding this comment

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

Used this JSON data in Block Kit Builder for verifying

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.81%. Comparing base (9976da9) to head (79fa8e2).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1337      +/-   ##
============================================
+ Coverage     74.77%   74.81%   +0.03%     
- Complexity     4177     4181       +4     
============================================
  Files           451      451              
  Lines         12930    12930              
  Branches       1331     1331              
============================================
+ Hits           9668     9673       +5     
+ Misses         2487     2483       -4     
+ Partials        775      774       -1     

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

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.

Great catches 💯

Left one comment worth addressing but it is none-blocking

@Builder
@NoArgsConstructor
@AllArgsConstructor
public static class NoCodeTextStyle {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this object may be referenced as limited_style, what do you think about naming this LimitedTextStyle?

Suggested change
public static class NoCodeTextStyle {
public static class LimitedTextStyle {

@@ -146,7 +150,23 @@ public static class TextStyle {
private boolean bold;
private boolean italic;
private boolean strike;
private boolean highlight;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 this makes sense, from what I was able to find there are currently only 2 versions of these style objects

@seratch seratch merged commit d226674 into slackapi:main Jul 16, 2024
4 checks passed
@seratch seratch deleted the rich-elements branch July 16, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-model project:slack-api-model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rich Text Block Mismatch in Java Class and Json Version
2 participants