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

chore: asset not found download status [WPB-4989] #2250

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

Garzas
Copy link
Contributor

@Garzas Garzas commented Nov 23, 2023


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?

  • added download status not found to avoid retry to download non existing asset's

Copy link
Contributor

github-actions bot commented Nov 23, 2023

Unit Test Results

   435 files   -   41     435 suites   - 41   25s ⏱️ - 2m 19s
2 459 tests  - 194  2 381 ✔️  - 168  78 💤  - 26  0 ±0 

Results for commit 0cbfedf. ± Comparison against base commit 23be862.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

Merging #2250 (0cbfedf) into develop (23be862) will decrease coverage by 0.02%.
The diff coverage is 20.93%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2250      +/-   ##
=============================================
- Coverage      57.93%   57.92%   -0.02%     
  Complexity        21       21              
=============================================
  Files           1084     1084              
  Lines          41084    41093       +9     
  Branches        3801     3804       +3     
=============================================
+ Hits           23803    23804       +1     
- Misses         15657    15662       +5     
- Partials        1624     1627       +3     
Files Coverage Δ
...tlin/com/wire/kalium/logic/data/message/Message.kt 49.13% <100.00%> (+0.17%) ⬆️
.../wire/kalium/network/exceptions/KaliumException.kt 79.24% <100.00%> (+0.39%) ⬆️
...ire/kalium/network/exceptions/NetworkErrorLabel.kt 100.00% <ø> (ø)
...re/kalium/persistence/dao/message/MessageEntity.kt 76.32% <100.00%> (+0.09%) ⬆️
...om/wire/kalium/logic/data/message/MessageMapper.kt 20.14% <0.00%> (ø)
...ire/kalium/logic/data/message/MessageRepository.kt 46.05% <0.00%> (ø)
...lium/logic/feature/asset/GetMessageAssetUseCase.kt 80.88% <60.00%> (-1.94%) ⬇️
...in/com/wire/kalium/logic/data/asset/AssetMapper.kt 42.36% <3.44%> (-0.60%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@datadog-wireapp
Copy link

datadog-wireapp bot commented Nov 23, 2023

Datadog Report

All test runs d6c7eec 🔗

2 Total Test Services: 0 Failed, 0 with New Flaky, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Wall Time Branch View
kalium-ios 0 0 0 2381 78 7m 20.1s Link
kalium-jvm 0 0 0 2549 104 7m 18.05s Link

Copy link
Contributor

@alexandreferris alexandreferris left a comment

Choose a reason for hiding this comment

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

🙏🏻

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Small comment, great job ! 🍸

@@ -116,7 +118,15 @@ internal class GetMessageAssetUseCaseImpl(
).fold({
kaliumLogger.e("There was an error downloading asset with id => ${assetMetadata.assetKey.obfuscateId()}")
// This should be called if there is an issue while downloading the asset
updateAssetMessageDownloadStatus(Message.DownloadStatus.FAILED_DOWNLOAD, conversationId, messageId)
if (it is NetworkFailure.ServerMiscommunication &&
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a CoreFailure.isInvalidRequestError helper, we might use I think

@Garzas Garzas enabled auto-merge November 23, 2023 10:38
@Garzas Garzas added this pull request to the merge queue Nov 24, 2023
Merged via the queue into develop with commit 6d61603 Nov 24, 2023
17 checks passed
@Garzas Garzas deleted the chore/asset-not-found branch November 24, 2023 06:13
augustocdias pushed a commit that referenced this pull request Dec 5, 2023
* chore: asset not found download status

* review fixes

* test fix
@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. 🚨 Potential breaking changes 👕 size: M type: chore 🧹
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants