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

APP-3073: Enhancements on MessageService #277

Merged
merged 18 commits into from
Oct 9, 2020

Conversation

symphony-hong
Copy link
Contributor

@symphony-hong symphony-hong commented Oct 7, 2020

Ticket

APP-3073

Description

Enhancements on message service
Move MessageService to package com.symphony.bdk.core.service.message

Make the TemplateException extends RuntimeException -> Remove all the throws TemplateException.

Rename MessageService#send(streamId, template, Object) -> MessageService#sendWithTemplate
Provide MessageService#send(streamId, template) to send a templated message without passing any parameter instead of passing null to the existing method.

Provide method MessageService#send(streamId, message, data, attachment) to send a message with data and attachment (streamId can be replaced by a V4Stream instance)

Small fix on webclient api to perform multipart/form-data request.

Unittest added

Dependencies

List the other pull requests that should be merged before/along this one.

Checklist

  • Referenced a ticket in the PR title and in the corresponding section
  • Filled properly the description and dependencies, if any
  • Unit tests updated or added
  • Javadoc added or updated
  • Updated the documentation in docs folder

User can set the privateKey and certificate content in bytes array to BdkConfig
for bot and app authentication.

Only one of path or content should be configured. If user configure both of them,
an AuthInitializationException will be thrown. The exception should be thrown at the step of
initializing the Authenticator instead of loading config because the BdkConfig can be set programmatically.

Unittest added
Move MessageService to package com.symphony.bdk.core.service.message

Make the TemplateException extends RuntimeException -> Remove all the throws TemplateException.

Rename MessageService#send(streamId, template, Object) -> MessageService#sendWithTemplate
Provide MessageService#send(streamId, template) to send a templated message without passing any parameter instead of passing null to the existing method.

Provide method MessageService#send(streamId, message, data, attachment) to send a message with data and attachment (streamId can be replaced by a V4Stream instance)

Small fix on webclient api to perform multipart/form-data request.

Unittest added
Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

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

I do have some concerns regarding breaking changes you've introduced here. Even if we are still in BETA, could we try to make a bit more smoother through deprecation mechanism?

We will clearly define the policy on next spring though (cf. APP-3114)

@symphony-hong
Copy link
Contributor Author

@symphony-thibault yes clearly this PR will produce some breaking changes as we are still in BETA. I also supposed to open a ticket to update the generator after this PR is finished and merged.

Copy link
Contributor

@symphony-youri symphony-youri left a comment

Choose a reason for hiding this comment

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

Make the TemplateException extends RuntimeException -> Remove all the throws TemplateException.
-> we see that from the change what would be interesting to state in the commit message is why :)? (the what is visible from the diff, the why is interesting to document in git logs so we can understand the decisions later when we look back at the code ;))

Adding two methods to send message passing a Message instance instead of many parameters
Adding ComplexMessageExampleMain.java to show the usage of the Message model.

These methods and example is under reviewing. The code will be refactored after review.
@symphony-hong
Copy link
Contributor Author

symphony-hong commented Oct 8, 2020

@symphony-thibault @symphony-youri I created a Message model to be used in the message service, added two methods using this model as well. (an example to see how to use it as well)
I still keep the old methods and will refactor the messsage service after we agree that the Message model is okay to use.

@@ -165,59 +170,67 @@ public TemplateEngine templates() {
* @return a {@link V4Message} object containing the details of the sent message
* @see <a href="https://developers.symphony.com/restapi/reference#create-message-v4">Create Message v4</a>
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Use Javadoc @deprecated tag to mention that this method will be replaced by send(V4Stream, Message)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should not we remove it at once since we are in beta?

@thibauult
Copy link
Member

thibauult commented Oct 8, 2020

I'd ❤️ to have:

final V4Message messageSent = bdk.message().builder()
    .template("/templates/welcome.ftl", values)
    .attachment(inputStream, "toto.txt") // or .attachment(byte[], "toto.txt") or .attachment(File)
    .data(singletonMap("key", "value"))
    .send(stream);

Knowing that message blast will be available soon in the Agent (agent-api-paths-public.yaml) we would also have:

final V4Message messageSent = bdk.message().builder()
    .template("/templates/welcome.ftl", values)
    .attachment(inputStream, "toto.txt")
    .data(singletonMap("key", "value"))
    .send(stream1, stream2, stream3);

@symphony-hong symphony-hong marked this pull request as ready for review October 9, 2020 13:01
@symphony-hong symphony-hong requested a review from a team October 9, 2020 13:01
@symphony-hong
Copy link
Contributor Author

@symphony-thibault is it exactly what you suggested? 865bf89#diff-1f276cf993a147eb9c7b918053efb18dL38

null,
tempFile,
Copy link
Member

Choose a reason for hiding this comment

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

this.createTemporaryAttachmentFile(message.getAttachment()) directly here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why yet but if I use this.createTemporaryAttachmentFile(message.getAttachment()), method will throw a RuntimeException instead of MessageCreationException and the unittest will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the retry throws every Throwable as RuntimeException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I would create the temp file outside the retry to avoid new file created if the retry recovery is triggered

Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

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

Good job 👍 Will be completed on next sprint with the ability to send multiple attachments, I'll create another ticket

@symphony-hong symphony-hong merged commit 4e33973 into finos:master Oct 9, 2020
@symphony-hong symphony-hong deleted the APP-3073 branch October 12, 2020 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants