Skip to content

Enable encrypted logging and add tracking#12779

Merged
malinajirka merged 3 commits intorelease/15.6from
issue/enable-encrypted-logging-with-tracking-2
Aug 24, 2020
Merged

Enable encrypted logging and add tracking#12779
malinajirka merged 3 commits intorelease/15.6from
issue/enable-encrypted-logging-with-tracking-2

Conversation

@oguzkocer
Copy link
Copy Markdown
Contributor

This PR re-enables encrypted logging and adds tracking. 738a934 is the revert commit, so it may be skipped during review, so the only code to review is 8a976ae.

To test:

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@oguzkocer oguzkocer added this to the 15.6 ❄️ milestone Aug 24, 2020
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Aug 24, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Aug 24, 2020

You can test the changes on this Pull Request by downloading the APK here.

@malinajirka malinajirka self-assigned this Aug 24, 2020
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks @oguzkocer! I'm going to test the changes now. Just dropping this comment to speed up the process in case it's actually a bug.

is EncryptedLogFailedToUpload -> {
AppLog.e(T.MAIN, "Encrypted log with uuid: ${event.uuid} failed to upload with error: ${event.error}")
// Only track final errors
if (event.willRetry) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to double check -> shouldn't this be !event.willRetry?

If it's correct the way it is now, I feel like the comment might be a bit confusing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that @malinajirka! Fixed in 574bbae.

Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks @oguzkocer! Works as expected ;)!

@malinajirka malinajirka merged commit 7b82938 into release/15.6 Aug 24, 2020
@malinajirka malinajirka deleted the issue/enable-encrypted-logging-with-tracking-2 branch August 24, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants