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

Article - Fix a crash when attempting to load a video with an invalid URL #725

Merged
merged 3 commits into from
May 9, 2023

Conversation

Gio2018
Copy link
Collaborator

@Gio2018 Gio2018 commented May 9, 2023

Summary

  • This PR fixes a crash occurring when trying to create a VideoComponent instance with an invalid url

References

  • See branch name

Implementation Details

  • Update VideoComponent, make the convenience initializer throw an error if the url from VideoParts is invalid
  • Update ArticleComponent, handle the errors described in the previous bullet

Test Steps

  • Write out what QA should do to verify this change works as expected and hasn't introduced regressions. Can mention specific OS versions, devices, areas of the app to test as needed.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

Screenshots

@Gio2018 Gio2018 force-pushed the fix/IN-1451-crash-invalid-video-url branch from a72a9e4 to e06aa47 Compare May 9, 2023 19:01
@@ -1,4 +1,6 @@
import Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need Foundation here?

Copy link
Collaborator Author

@Gio2018 Gio2018 May 9, 2023

Choose a reason for hiding this comment

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

Yes (URL)
Actually, good catch! It's a leftover 😄

Copy link
Contributor

@cyndichin cyndichin left a comment

Choose a reason for hiding this comment

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

looks great! I am able to see the article and the app doesn't crash with this change. thanks! will give it another look when ready to review

@pocket-ci
Copy link
Contributor

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 76.64%
📖 Checking XCode Environment Variables
📖 Edited 2 files
📖 Created 0 files

Sync: Coverage: 84.56

File Coverage
ArticleComponent.swift 96.17%
VideoComponent.swift 94.44%

Generated by 🚫 Danger Swift against e06aa47

@Gio2018 Gio2018 changed the title Fix/in 1451 crash invalid video url Article - Fix a crash when attempting to load a video with an invalid URL May 9, 2023
@Gio2018 Gio2018 self-assigned this May 9, 2023
@Gio2018 Gio2018 added bug Something isn't working marticle labels May 9, 2023
@Gio2018 Gio2018 marked this pull request as ready for review May 9, 2023 20:10
@Gio2018 Gio2018 requested a review from a team as a code owner May 9, 2023 20:10
@Gio2018 Gio2018 requested review from cyndichin and timc-mozilla and removed request for a team May 9, 2023 20:10
@Gio2018 Gio2018 enabled auto-merge (rebase) May 9, 2023 20:10
@Gio2018 Gio2018 merged commit 166f85d into develop May 9, 2023
@Gio2018 Gio2018 deleted the fix/IN-1451-crash-invalid-video-url branch May 9, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working marticle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants