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.

Sign up for GitHub

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

Jump to bottom

[MOB-15817] Add getUrlVariables public API #59

Merged
merged 17 commits into from
May 13, 2022
Merged

[MOB-15817] Add getUrlVariables public API #59

merged 17 commits into from
May 13, 2022

Conversation

addb
Copy link
Contributor

@addb addb commented Apr 20, 2022

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@addb addb requested review from emdobrin and kevinlind April 20, 2022 05:01
@codecov
Copy link

codecov bot commented Apr 20, 2022 •
edited
Loading

Codecov Report

Merging #59 (890ee09) into feature/getUrlVariables (9bf6486) will increase coverage by 0.28%.
The diff coverage is 81.25%.

@@                     Coverage Diff                     @@
##           feature/getUrlVariables      #59      +/-   ##
===========================================================
+ Coverage                    80.51%   80.79%   +0.28%     
===========================================================
  Files                           18       19       +1     
  Lines                          826      937     +111     
  Branches                       125      136      +11     
===========================================================
+ Hits                           665      757      +92     
- Misses                         115      130      +15     
- Partials                        46       50       +4
Flag Coverage Δ
functional-tests 58.91% <56.25%> (-0.53%) ⬇️
unit-tests 79.08% <76.79%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...keting/mobile/edge/identity/IdentityConstants.java 0.00% <0.00%> (ø)
...eting/mobile/edge/identity/IdentityProperties.java 90.16% <ø> (ø)
...adobe/marketing/mobile/edge/identity/URLUtils.java 80.95% <80.95%> (ø)
...adobe/marketing/mobile/edge/identity/Identity.java 72.31% <81.25%> (+2.92%) ⬆️
...keting/mobile/edge/identity/IdentityExtension.java 76.33% <85.29%> (+3.54%) ⬆️
...obe/marketing/mobile/edge/identity/EventUtils.java 67.35% <85.71%> (+13.78%) ⬆️
...om/adobe/marketing/mobile/edge/identity/Utils.java 77.42% <100.00%> (+0.37%) ⬆️

@addb addb changed the base branch from dev to feature/getUrlVariables April 21, 2022 01:44
kevinlind
kevinlind reviewed Apr 21, 2022
View reviewed changes
code/app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Identity.java Outdated Show resolved Hide resolved
...eidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityExtensionTests.java Outdated Show resolved Hide resolved
code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityTests.java Show resolved Hide resolved
code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityTests.java Show resolved Hide resolved
code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityTests.java Show resolved Hide resolved
code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityTests.java Show resolved Hide resolved
@kevinlind
Copy link
Collaborator

In the review files, Codecov annotates lines which do not have any test coverage. Can you increase the diff coverage by adding tests which cover those lines?

kevinlind
kevinlind reviewed Apr 22, 2022
View reviewed changes
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Identity.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Identity.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java Outdated Show resolved Hide resolved
code/edgeidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/URLUtilsTests.java Outdated Show resolved Hide resolved
kevinlind
kevinlind approved these changes Apr 23, 2022
View reviewed changes
Copy link
Collaborator

@kevinlind kevinlind left a comment

Choose a reason for hiding this comment

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

Can you update the method comment for the EventUtils method you changed before merging. Otherwise, looks good.

code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
emdobrin
emdobrin reviewed May 11, 2022
View reviewed changes
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Identity.java Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/EventUtils.java Show resolved Hide resolved
...eidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityExtensionTests.java Show resolved Hide resolved
...eidentity/src/test/java/com/adobe/marketing/mobile/edge/identity/IdentityExtensionTests.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/Utils.java Outdated Show resolved Hide resolved
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/URLUtils.java Outdated Show resolved Hide resolved
@emdobrin emdobrin merged commit f260058 into adobe:feature/getUrlVariables May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinlind kevinlind kevinlind approved these changes

@emdobrin emdobrin emdobrin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@addb @kevinlind @emdobrin