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.

Sign up for GitHub

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

Jump to bottom

Fix Channels not showing up when Message attachment has malformed data #834

Merged
merged 4 commits into from Feb 22, 2021
Merged

Fix Channels not showing up when Message attachment has malformed data #834

merged 4 commits into from Feb 22, 2021

Conversation

DominikBucher12
Copy link
Contributor

What this PR do:

  • Fixes issue where malformed attachment data could cause Channel not to appear

How to test it

  1. Open 2 simulator device at same time and on each run the same instance of Demo App
  2. Select any user
  3. Exit one of the simulators (A)
  4. From the other one (B) send some messages for the selected user account
  5. Start A again with the app and log in to the same user
  6. Observe that all channels are displayed for the same user on both devices

@DominikBucher12 DominikBucher12 changed the title Fix Message attachment malformed data Fix Channels not showing up when Message attachment has malformed data Feb 22, 2021
@codecov
Copy link

codecov bot commented Feb 22, 2021 •
edited

Codecov Report

Merging #834 (900c74d) into main (8903928) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   86.70%   86.74%   +0.03%     
==========================================
  Files         228      229       +1     
  Lines        8638     8641       +3     
==========================================
+ Hits         7490     7496       +6     
+ Misses       1148     1145       -3
Impacted Files Coverage Δ
...IClient/Endpoints/Payloads/AttachmentPayload.swift 100.00% <100.00%> (ø)
...APIClient/Endpoints/Payloads/MessagePayloads.swift 100.00% <100.00%> (ø)
...urces/StreamChat/Database/DTOs/AttachmentDTO.swift 67.47% <100.00%> (+0.49%) ⬆️
...treamChat/Models/Attachments/AttachmentTypes.swift 89.24% <100.00%> (-0.12%) ⬇️
...els/Attachments/ChatMessageDefaultAttachment.swift 96.22% <100.00%> (ø)
Sources/StreamChat/Utils/OptionalDecodable.swift 100.00% <100.00%> (ø)
.../StreamChatTests/DummyData/AttachmentPayload.swift 70.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8903928...4531da3. Read the comment docs.

VojtaStavik
VojtaStavik approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@VojtaStavik VojtaStavik left a comment

Choose a reason for hiding this comment

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

Please also add a test for the new functionality. Thanks

Sources/StreamChat/Database/DTOs/MessageDTO.swift Outdated Show resolved Hide resolved
nuno-vieira
nuno-vieira approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

VojtaStavik
VojtaStavik reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@VojtaStavik VojtaStavik left a comment

Choose a reason for hiding this comment

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

Just nits :shipit:

Sources/StreamChat/Utils/OptionalDecodable.swift

import Foundation

struct OptionalDecodable<Base: Decodable>: Decodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining what is this and why/when to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Sources/StreamChat/APIClient/Endpoints/Payloads/MessagePayloads.swift Outdated
Comment on lines 95 to 97
attachments = try container.decode([AttachmentPayload].self, forKey: .attachments)
attachments = try container.decode([OptionalDecodable<AttachmentPayload>].self, forKey: .attachments).compactMap(\.base)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining what is happening 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.

Fixed.

Sources/StreamChat/APIClient/Endpoints/Payloads/MessagePayloads_Tests.swift Show resolved Hide resolved
@VojtaStavik VojtaStavik self-requested a review February 22, 2021 16:07
b-onc
b-onc approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@b-onc b-onc left a comment

Choose a reason for hiding this comment

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

I like the solution, pity that this isn't JSONDecoder's default behavior....

@Stream-iOS-Bot
Copy link
Collaborator

1 Error
🚫 Please use more than one word.
900c74d

Generated by 🚫 Danger

@VojtaStavik VojtaStavik merged commit b36235a into main Feb 22, 2021
@VojtaStavik VojtaStavik deleted the Fix_AttachmentDTO_saving branch February 22, 2021 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-onc b-onc b-onc approved these changes

@VojtaStavik VojtaStavik VojtaStavik approved these changes

@nuno-vieira nuno-vieira nuno-vieira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DominikBucher12 @Stream-iOS-Bot @b-onc @VojtaStavik @nuno-vieira