3887 – Better error handling in PermaCC #412
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While working on refactorinf the tests: "At first the mocked test seemed to pass, but it was failing because I wasn't mocking the response correct so response.message was nil. After I mocked it correctly it stopped adding it to data. That was happening because we were raising the first error as RetryLater and we don't set the data when we rescue from RetryLater, only StandardError (media_archiver.rb:93)." Besides that, we don't want to retry if the user's perma_cc usage limit is hit.
we don't want TooManyCaptures to raise an error and be retried, but we do want the data to be updated with the error
we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket
|
Code Climate has analyzed commit bc99a6a and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 96.6%. View more on Code Climate. |
caiosba
reviewed
Oct 26, 2023
| raise Pender::Exception::RetryLater, "(#{response.code}) #{response.message}" | ||
| data = { error: { message: response.message, code: Lapis::ErrorCodes::const_get('ARCHIVER_ERROR') }} | ||
| Media.notify_webhook_and_update_cache('perma_cc', url, data, key_id) | ||
| if response&.body.include?("You've reached your usage limit") |
There was a problem hiding this comment.
It would be good to have an integration test for this case, just for us to capture if they change their message. On the other hand, this is hard because we would need to have a dedicated account to use only for this test. Hopefully we can track that in the generic Pender::Exception::PermaCcError exception report. So let's not change anything for now, this is good to go.
caiosba
approved these changes
Oct 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We want PermaCC to have specific errors, like ArchiveOrg, so we can better track and handle errors. So I created PermaCcError, and it's also using TooManyCaptures once we hit it's limit.
Importat: While working on tests I noticed that we were raising RetryLater when we errored in PermaCC, and that was causing it to behave in a different way then expected (it wasn't updating data and notifying the webhook, that would only happen when we hit the retry limit). So we now raise PermaCcError. It's retried, updates data and notifies the webhook.
Besides that, we don't want to retry when the archiving limit is hit. So when we hit TooManyCaptures it updates data and the webhook, but does not retry.
CV2-3886
How has this been tested?
test "when Perma.cc fails with Pender::Exception::PermaCcError it should update media with error and retry"test "when Perma.cc fails with Pender::Exception::TooManyCaptures it should update media with error and not retry"Things to pay attention to during code review
Not sure if it's something to pay attention, but it's something to note: I skipped the video archiving tests (since we are not using that feature anymore), hoping it would make the tests a bit faster but it didn't make much of a difference 😅 (the tests on the refactoring branch are much faster ❤️ )