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

fix: Parse system info from root if no nested system dictionary exists #296

Merged
merged 1 commit into from
Jul 17, 2018

Conversation

kattrali
Copy link
Contributor

In reports, the base report has a “system” dict for system info.

Sessions rely on the raw info parser to gather app and device data,
which is not nested in a separate “system” dict.

Goal

The changes in #292 fixed sending a custom version for reports but
broke app version parsing for sessions. This change refactors app
state parsing slightly to ensure both reports and sessions fetch the
correct app state (and custom versions, if provided).

Changeset

Moved the top level version parsing out to error reports from BSGParseAppState. The preferred app version is now (optionally) specified up front.

- BSGParseAppState(errorReport)
+ BSGParseAppState(sysInfo, preferredVersion)

Tests

Added new integration test cases for verifying app version with or without custom versions set.

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@kattrali kattrali requested a review from a team July 16, 2018 20:19
Copy link

@martin308 martin308 left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment on the use of constants


BSGDictSetSafeObject(app, system[@"CFBundleVersion"], @"bundleVersion");
BSGDictSetSafeObject(app, report[@"CFBundleVersion"], @"bundleVersion");

Choose a reason for hiding this comment

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

should bundleVersion be a const like the other keys? BSGKeyReleaseStage etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe; I want to think about how we are doing key constants a bit in the meantime in case there's an easy way to both define and find the right ones.

In reports, the base report has a “system” dict for system info.
Sessions rely on the raw info parser to gather app and device data,
which is not nested in a separate “system” dict.

Added additional tests for app versions
@kattrali kattrali force-pushed the kattrali/session-verify-app-version branch from 70d16ff to 861b493 Compare July 17, 2018 08:41
@kattrali kattrali merged commit 261c111 into master Jul 17, 2018
@kattrali kattrali deleted the kattrali/session-verify-app-version branch July 17, 2018 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants