Add method for Slack API's migration.exchange endpoint. #235
Conversation
There was a problem hiding this comment.
LGTM
@omotnyk just asking you to have another look at this in case I've missed something. Thanks!
|
Taking a look now 👀 |
...c/main/java/com/hubspot/slack/client/methods/params/migration/MigrationExchangeParamsIF.java
Outdated
Show resolved
Hide resolved
...ain/java/com/hubspot/slack/client/models/response/migration/MigrationExchangeResponseIF.java
Outdated
Show resolved
Hide resolved
...ain/java/com/hubspot/slack/client/models/response/migration/MigrationExchangeResponseIF.java
Show resolved
Hide resolved
… param name and add jsonProperty to correspond slack's expected json property name
|
Yes, I'm adding them. Will push them in a while. |
… response and params.
|
Updated. |
|
|
||
| Map<String, String> getUserIdMap(); | ||
|
|
||
| Optional<Set<String>> getInvalidUserIds(); |
There was a problem hiding this comment.
One more thing. I wonder if it's easier to have Set<String> instead of Optional here. Set<String> getInvalidUserIds will be parsed as an empty set if no property is present in JSON at all. So we still can work with the entity but IMHO, it's a bit easier to work with a set that can be empty than with Optional. What do you think?
There was a problem hiding this comment.
Yes that will be better. I just didn't now that json can deserialize without exception. when set property is missing. I'll update it.
There was a problem hiding this comment.
Now that we've removed Optional, I think it's okay to rewrite the tests to make them easier to read/understand. I suggest using AssertJ for assertions and the lib is already connected to this project.
Overall, LGTM. Please, update the tests if you think my comments make sense and feel free to merge without further approval.
Thank you!
| private boolean doesMainParamsPresent(MigrationExchangeResponse migrationExchangeResponse) { | ||
| return migrationExchangeResponse.isOk() && | ||
| !migrationExchangeResponse.getTeamId().isEmpty() && | ||
| !migrationExchangeResponse.getEnterpriseId().isEmpty(); | ||
| } |
There was a problem hiding this comment.
I think it makes sense to change this to
private void assertMainResponseFields(MigrationExchangeResponse migrationExchangeResponse) {
assertThat(migrationExchangeResponse.isOk()).isTrue();
assertThat(migrationExchangeResponse.getTeamId()).isEqualTo("T024G0P55");
assertThat(migrationExchangeResponse.getEnterpriseId()).isEqualTo("E01L6J4CPS8");
}This way, we verify each field separately and will receive more meaningful error in case of failure. Also, we'll verify that the values from the json fixture are parsed correctly instead of simply checking that they are present.
| MigrationExchangeResponse migrationExchangeResponse = | ||
| fetchAndDeserializeMigrationExchangeResponse("migration_exchange_response_with_valid_and_invalid_user_ids.json"); | ||
| Map<String, String> userIdMap = migrationExchangeResponse.getUserIdMap(); | ||
| Set<String> invalidUserIds = migrationExchangeResponse.getInvalidUserIds(); | ||
| assertTrue(doesMainParamsPresent(migrationExchangeResponse)); | ||
| assertTrue(userIdMap.containsKey(VALID_USER_OLD_ID) && userIdMap.containsValue(VALID_USER_MIGRATED_ID)); | ||
| assertTrue(invalidUserIds.contains(INVALID_USER_ID)); |
There was a problem hiding this comment.
This looks a bit better to me:
assertMainResponseFields(migrationExchangeResponse);
assertThat(userIdMap).containsKey(VALID_USER_OLD_ID);
assertThat(userIdMap).containsValue(VALID_USER_MIGRATED_ID);
assertThat(invalidUserIds).containsExactly(INVALID_USER_ID);What do you think?
There was a problem hiding this comment.
Yes with assertThat it will return more precise errors.
| assertTrue(doesMainParamsPresent(migrationExchangeResponse)); | ||
| assertTrue(userIdMap.containsKey(VALID_USER_OLD_ID) && userIdMap.containsValue(VALID_USER_MIGRATED_ID)); | ||
| assertTrue(invalidUserIds.isEmpty()); |
There was a problem hiding this comment.
This looks a bit better to me:
assertMainResponseFields(migrationExchangeResponse);
assertThat(userIdMap).containsKey(VALID_USER_OLD_ID);
assertThat(userIdMap).containsValue(VALID_USER_MIGRATED_ID);
assertThat(invalidUserIds).isEmpty();What do you think?
| assertTrue(doesMainParamsPresent(migrationExchangeResponse)); | ||
| assertTrue(userIdMap.isEmpty()); | ||
| assertTrue(invalidUserIds.contains(INVALID_USER_ID)); |
There was a problem hiding this comment.
This looks a bit better to me:
assertMainResponseFields(migrationExchangeResponse);
assertThat(userIdMap).isEmpty();
assertThat(invalidUserIds).containsExactly(INVALID_USER_ID);What do you think?
| import com.hubspot.slack.client.models.JsonLoader; | ||
| import com.hubspot.slack.client.models.response.migration.MigrationExchangeResponse; | ||
|
|
||
| public class MigrationExchangeParamsDeserializationTest { |
There was a problem hiding this comment.
The first three comments in this file depend on the last one.
There was a problem hiding this comment.
Also, you'll need to use import static org.assertj.core.api.Assertions.assertThat; in order to change the assertions as I suggested.
…in place of junit
|
🚢 |
|
I updated the test to use there assertj according to comments. Will merge it. |
higityan
deleted the
add_method_for_slack_api_migration_exchange_endpoint
branch
Add method for Slack API's migration.exchange endpoint. After migration slack ids has changed, this endpoint returns map of old to new ids.