Skip to content

Conversation

@roninopf
Copy link
Contributor

@roninopf roninopf commented Dec 17, 2021

🔹 Jira Ticket(s) if any

✏️ Description

  • implements allowedProtocols into IterableConfig and makes sure that links that aren't https or in that list are not opened by the SDK

@roninopf roninopf requested a review from Ayyanchira December 17, 2021 01:57
Test Fix for PR - Allowed Protocols
@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #411 (f65950e) into master (5f38aad) will decrease coverage by 0.36%.
The diff coverage is 29.41%.

❗ Current head f65950e differs from pull request most recent head c3d5543. Consider uploading reports for the commit c3d5543 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
- Coverage   67.47%   67.11%   -0.37%     
==========================================
  Files          63       63              
  Lines        3806     3822      +16     
  Branches      440      445       +5     
==========================================
- Hits         2568     2565       -3     
- Misses        977      995      +18     
- Partials      261      262       +1     
Impacted Files Coverage Δ
...com/iterable/iterableapi/IterableActionRunner.java 48.57% <0.00%> (-2.95%) ⬇️
...ain/java/com/iterable/iterableapi/IterableApi.java 65.02% <ø> (+0.25%) ⬆️
.../iterable/iterableapi/IterableDeeplinkManager.java 53.22% <0.00%> (-1.78%) ⬇️
...ableapi/IterableInAppFragmentHTMLNotification.java 48.90% <ø> (ø)
...com/iterable/iterableapi/IterableInAppManager.java 88.57% <ø> (+0.47%) ⬆️
...rable/iterableapi/IterableNotificationBuilder.java 81.25% <ø> (ø)
...erable/iterableapi/IterableNotificationHelper.java 76.83% <ø> (ø)
...in/java/com/iterable/iterableapi/IterableUtil.java 62.50% <25.00%> (-8.04%) ⬇️
.../java/com/iterable/iterableapi/IterableConfig.java 76.74% <50.00%> (-2.75%) ⬇️
...java/com/iterable/iterableapi/IterableWebView.java 100.00% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0a694a...c3d5543. Read the comment docs.

Copy link
Contributor

@davidtruong davidtruong left a comment

Choose a reason for hiding this comment

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

Added comments

@Ayyanchira Ayyanchira mentioned this pull request Dec 22, 2021
1. InAppManager no more holds array of allowedProtocols. Its always accessed from IterableConfig.
2. `isUrlOpenAllowed` is now a common utility method in IterableUtil
3. Modified test methods to accomodate the changes
1. Only place to check if the url is allowed is through the Util method which checks the allowedProtocols list from config.
2. Removed method signatures that were introduced because of this
3. Test methods update
4. Logs if the url was blocked
*
* @param context Context
* @param action The original action object
* @param allowedProtocols protocols that the SDK is allowed to open in addition to `https`
Copy link
Member

Choose a reason for hiding this comment

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

Have to remove this

@Ayyanchira Ayyanchira force-pushed the jay/MOB-3769-allowed-protocols branch from 97f21a4 to 4e58592 Compare December 22, 2021 23:42
Copy link
Contributor

@davidtruong davidtruong left a comment

Choose a reason for hiding this comment

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

LFTM

…cols

Mob 3850 patch for allowed protocols
@Ayyanchira Ayyanchira force-pushed the jay/MOB-3769-allowed-protocols branch from 4e58592 to c3d5543 Compare December 22, 2021 23:55
@Ayyanchira Ayyanchira merged commit 873708f into master Dec 23, 2021
@Ayyanchira Ayyanchira deleted the jay/MOB-3769-allowed-protocols branch December 23, 2021 16:55
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.

4 participants