Skip to content
This repository was archived by the owner on Mar 28, 2022. It is now read-only.

SYM-1613 - Add ability to attach multiple attachements on client messages#87

Merged
symphony-elias merged 3 commits into
SymphonyPlatformSolutions:masterfrom
JulienBlacas:feat/msg-multiple-attachements
Nov 27, 2020
Merged

SYM-1613 - Add ability to attach multiple attachements on client messages#87
symphony-elias merged 3 commits into
SymphonyPlatformSolutions:masterfrom
JulienBlacas:feat/msg-multiple-attachements

Conversation

@JulienBlacas

Copy link
Copy Markdown
Contributor

This PR is meant to give the developper the ability to send multiple attachements inside one message

@JulienBlacas JulienBlacas requested a review from a team July 9, 2020 07:40
conversationId,
message,
data,
files,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

does this have any public documentation?
we should be clair on the format of the files array

@symphony-adnane

Copy link
Copy Markdown

Just some open comments
Thanks Julien for adding this capacity, it's very needed
May you please attach the user story reference to help us understand better the context (it's always good to have some extra info)
I don't see any unit tests on this change, which scares a bit the shit out of me, probably we didn't have it setup already, but we should thing about it
I don't see any way to document the method signatures, we should think about using typescript so we could create definition files automatically and help developers

I'm adding these notes here just so we are aware of these improvements,
Thanks again

symphony-adnane
symphony-adnane previously approved these changes Jul 9, 2020
@JulienBlacas

Copy link
Copy Markdown
Contributor Author

@symphony-adnane You are correct, typescript would help a lot in this situation . For the moment, we could think of a validation function that check if the files parameter is sent with the proper scheme. If this is not so, we can return an Error which explain how the files array should be structured. For more context about our user story here is the corresponding JIRA ticket : SYM-1613

@symphony-adnane

Copy link
Copy Markdown

@symphony-adnane You are correct, typescript would help a lot in this situation . For the moment, we could think of a validation function that check if the files parameter is sent with the proper scheme. If this is not so, we can return an Error which explain how the files array should be structured. For more context about our user story here is the corresponding JIRA ticket : SYM-1613

Sounds good, we'll work midterm on supporting typescript.
Could you btw add some unit tests for this, I saw that jest is already setup! thanks

@symphony-adnane symphony-adnane changed the title Add ability to attach multiple attachements on client messages SYM-1613 - Add ability to attach multiple attachements on client messages Jul 9, 2020
@JulienBlacas

Copy link
Copy Markdown
Contributor Author

@symphony-adnane Yes I will add some unit tests and keep you posted

@symphony-elias symphony-elias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good !
Successfully tested on devx3

@symphony-elias symphony-elias merged commit 3584130 into SymphonyPlatformSolutions:master Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants