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

fix: bug not downloading correctly asset messages from web client #413

Merged
merged 7 commits into from
Apr 19, 2022

Conversation

gongracr
Copy link
Contributor


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

When Web client sends asset messages, it does it with 2 messages:

  1. First it sends a preview asset message, with the file metadata information such as asset width and height (in case of Images or video), file name, mimeType, and asset size. This is in case the other receiving client wanted to just show a quick preview placeholder with that metadata.
  2. Secondly it sends another asset message with the same message and conversation ids than the first one, but just containing the asset RemoteData (decryption keys, asset id and asset token)

The way that the asset send and receive logic was implemented previously, was discarding any asset message that didn't include the file decryption keys. Therefore file metadata could not be displayed correctly if the original message didn't contain both.

Solutions

Even though in AR we send now just 1 asset message with all the needed info (metadata + decryption keys), to allow retrocompatibility with web clients that implement above behavior we had to make a workaround. This consists on storing the first preview message, and then updating it accordingly once the second message arrives with the final decryption keys

How to Test

A PR in AR will be raised following this one containing the fix.

Notes (Optional)

This solution is not optimal due to the fact that there is a lapse of time (between the 1st and 2nd messages being received) where the asset message received will not be able to be downloaded, as the assetId will be missing temporarily.


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2022

Unit Test Results

  92 files    92 suites   44s ⏱️
413 tests 407 ✔️ 6 💤 0

Results for commit 3dd5df0.

♻️ This comment has been updated with latest results.

@typfel
Copy link
Member

typfel commented Apr 19, 2022

Did you consider ignoring asset messages which doesn't contain RemoteData?

@gongracr
Copy link
Contributor Author

@typfel yes, that's what was being doine previously, and that caused that the asset messages appeared to be empty as those were the only ones including the asset metadata such as asset file name and size
Captura de Pantalla 2022-04-18 a las 18 00 22

@typfel
Copy link
Member

typfel commented Apr 19, 2022

@typfel yes, that's what was being doine previously, and that caused that the asset messages appeared to be empty as those were the only ones including the asset metadata such as asset file name and size
Captura de Pantalla 2022-04-18 a las 18 00 22

I mean we don't persist the message at all if the protobuf doesn't contain RemoteData. Maybe adding a check in ConversationEventReceiver.

Copy link
Member

@typfel typfel left a comment

Choose a reason for hiding this comment

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

👍 and I would consider filtering out asset messages which only has the preview.

@gongracr gongracr merged commit f89a7df into develop Apr 19, 2022
@gongracr gongracr deleted the feat/add_download_asset_flow branch April 19, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants