Skip to content

[FIX] Apps converters delete fields on message attachments#14028

Merged
rodrigok merged 27 commits intodevelopfrom
fix-deletion-of-field-during-app-conversion
Apr 16, 2019
Merged

[FIX] Apps converters delete fields on message attachments#14028
rodrigok merged 27 commits intodevelopfrom
fix-deletion-of-field-during-app-conversion

Conversation

@d-gubert
Copy link
Member

@d-gubert d-gubert commented Apr 6, 2019

Whenever a message is sent over to the apps engine to trigger a hook, the message converter that bridges the data from Rocket.Chat to the Engine does not take all the fields from the original object to the converted one.

When data comes back from the Engine, it was not being merged back to the message object properly, which was causing the attachments (and any other non-primitive value) to lose some of the fields.

With these changes, fields that are ignored by the Engine will no longer be deleted from the message's attachments.

Closes #12691

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 6, 2019 21:41 Inactive
@d-gubert d-gubert requested a review from sampaiodiego April 6, 2019 21:47
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 6, 2019 22:56 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 7, 2019 01:56 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 7, 2019 15:54 Inactive
@rodrigok rodrigok self-assigned this Apr 7, 2019
@rodrigok
Copy link
Member

rodrigok commented Apr 7, 2019

@d-gubert how this handle fields removal? If I unset, like, attachments from inside the engine?

@rodrigok
Copy link
Member

rodrigok commented Apr 7, 2019

@d-gubert how this handle fields removal? If I unset, like, attachments from inside the engine?

This may answer my question:
https://github.com/RocketChat/Rocket.Chat/pull/14028/files#diff-c961b69f32e88f90d70fea25dc2ae0b2R4

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 7, 2019 20:43 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-14028 April 7, 2019 20:48 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 7, 2019 23:51 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 7, 2019 23:57 Inactive
@engelgabriel engelgabriel added this to the 1.0.0 milestone Apr 8, 2019
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 8, 2019 18:56 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 9, 2019 15:02 Inactive
@d-gubert
Copy link
Member Author

d-gubert commented Apr 9, 2019

Discussing with @sampaiodiego and @tassoevan we found a more elegant solution. I'll be closing this PR for now until I have implemented it.

@d-gubert d-gubert reopened this Apr 10, 2019
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 10, 2019 14:50 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 10, 2019 15:11 Inactive
@d-gubert d-gubert requested a review from rodrigok April 10, 2019 15:12
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 10, 2019 19:36 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 10, 2019 19:42 Inactive
@rodrigok
Copy link
Member

The code looks good, just missing some unit tests 😄

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 13, 2019 01:56 Inactive
@d-gubert
Copy link
Member Author

Added some tests.

Some of the mocks are kind of generic and maybe could be moved to a better place? What do you think @rodrigok? 🤔

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 13, 2019 13:16 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 15, 2019 15:58 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14028 April 15, 2019 18:38 Inactive
@rodrigok rodrigok merged commit 270f513 into develop Apr 16, 2019
@rodrigok rodrigok deleted the fix-deletion-of-field-during-app-conversion branch April 16, 2019 14:08
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
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.

pretext in message attachments does not get displayed

3 participants

Comments