HubSpot / slack-client

An asynchronous HTTP client for Slack's web API
Apache License 2.0
114 stars 53 forks source link

Reclaim contributions - all #166

Closed lightbody closed 4 years ago

lightbody commented 4 years ago

Hey there - sorry I've been MIA the last few months. Just heads down with our startup and hadn't had a chance to circle back on a couple PRs, plus some new stuff I recently needed.

Rather than letting them linger in my fork for months and get behind, I'm dropping it all here as one PR so that worst case they are on the record. I'll see if I can submit separate ones for each individual improvement, but given my spotty record who knows 🤣

First, some notes about the two existing PRs that this one encapsulates:

PR #145 is here, but also with the move from HomeTabViewResponseIF to HomeTabViewResponse per the comment there by @szabowexler.

PR #134 is also here, with no meaningful changes. I haven't had a chance to resolved the question of testing. The issue I bumped into is that the existing tests in the suite, which I would normally crib from, seem to rely on serializing and deserializing and then comparing the JSON tree. The problem here is that BlockElementActionIF returns a String for getSelectedValue() but the actual JSON payload can sometimes be a string and other times be a complex object representing a "selected option". So the serialization going back ends up different, causing the test to fail. I haven't been able to spend enough time to come up with a more appropriate test for this class.

OK, and now the new stuff captured in here:

Apologies for dumping them in a group like this. By far the biggest inhibitor to being a better contributor is my noob-level git skills. I hope to borrow a teammate in the coming days and enlist his help in breaking these out and squashing them into a single commit, but in the meantime I wanted to get this on the record.

lightbody commented 4 years ago

I just added to this PR a few more things:

Alternative to InteractiveLoadOptionsRequestIF

An alternative set of classes for handling incoming callbacks for dynamic external select menus. It seems that the documentation here https://api.slack.com/legacy/interactive-message-field-guide#options_load_url is captured by InteractiveLoadOptionsRequestIF

But what I'm finding is that when interacting this type of element from BlockKit-based UI (views, modals, messages) I get a very different payload that is undocumented. The BlocksLoadOptionsRequestIF class is an attempt to ease serializing that alternative payload.

I raised this issue with Slack here: https://twitter.com/plightbo/status/1266113801390190592?s=21

Support for selected_date for BlockElementActionIF

One of the other patches in this PR includes selected_option, but now I needed selected_date, so this was added to BlockElementActionIF and the getSelectedValue() field (which was a required String) is now Optional because you can have a non-String payload (ex: LocalDate).

A couple tests

Some simple serialization tests for BlockElementActionIF selected_option and selected_date. I still need to add some tests for the new InteractiveLoadOptionsRequestIF class, but I'll wait until I hear if you're open to that contribution before bothering.