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

Add support for hasUnmappedDestinations #905

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Add support for hasUnmappedDestinations #905

merged 2 commits into from
Dec 8, 2023

Conversation

zikaari
Copy link
Contributor

@zikaari zikaari commented Dec 7, 2023

Implements the spec as explained here

Parity established with segment-integrations/analytics-swift-consent@6563d95

@zikaari zikaari requested a review from oscb December 7, 2023 17:45
.every(([_, v]) => this.containsConsentSettings(v))
)
!this.isConsentFeatureSetup() ||
!(noneConsented && !this.hasUnmappedDestinations())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silesky @oscb I'm tunnel visioned, can you folks help me simplify line 94? I'm sure there's a way to not have these nested negations, just can't figure how??

Note that if expression on line 91 resolves to true, then that means event is allowed to flow, if false it is blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the isConsentFeatureSetup is an interesting one to have here. Cause potentially it might throw a "false" positive and let all events flow if the settings object is not loaded (f.e. you have bad connection). I guess this is more of an scenario where we should let the client control what they want to do in that scenario. Not sure really if this has been talked in Consent Management before. Anyway. We don't have to solve this yet.

I think the line !(noneConsented && !this.hasUnmappedDestinations()) could be rewritten as !noneConsented || !this.hasUnmappedDestinations(). If I'm understanding correctly the spec you want to let things flow if everything is consented or when there's at least an unmapped destination.

Copy link
Contributor

Choose a reason for hiding this comment

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

another idea to avoid many negations which make it harder to understand is to swap noneConsented to allConsented so that you don't have to negate.

Copy link
Contributor

@oscb oscb left a comment

Choose a reason for hiding this comment

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

looks good! left an idea for how to simplify the boolean logic

.every(([_, v]) => this.containsConsentSettings(v))
)
!this.isConsentFeatureSetup() ||
!(noneConsented && !this.hasUnmappedDestinations())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the isConsentFeatureSetup is an interesting one to have here. Cause potentially it might throw a "false" positive and let all events flow if the settings object is not loaded (f.e. you have bad connection). I guess this is more of an scenario where we should let the client control what they want to do in that scenario. Not sure really if this has been talked in Consent Management before. Anyway. We don't have to solve this yet.

I think the line !(noneConsented && !this.hasUnmappedDestinations()) could be rewritten as !noneConsented || !this.hasUnmappedDestinations(). If I'm understanding correctly the spec you want to let things flow if everything is consented or when there's at least an unmapped destination.

.every(([_, v]) => this.containsConsentSettings(v))
)
!this.isConsentFeatureSetup() ||
!(noneConsented && !this.hasUnmappedDestinations())
Copy link
Contributor

Choose a reason for hiding this comment

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

another idea to avoid many negations which make it harder to understand is to swap noneConsented to allConsented so that you don't have to negate.

Copy link
Contributor

@oscb oscb left a comment

Choose a reason for hiding this comment

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

looks good! left an idea for how to simplify the boolean logic

@zikaari
Copy link
Contributor Author

zikaari commented Dec 8, 2023

Had a discussion with Seth on this, lets keep the expressions as-is. They are compliant with the spec.

@zikaari zikaari merged commit 017c5ef into master Dec 8, 2023
4 checks passed
@zikaari zikaari deleted the conset_part_3 branch December 8, 2023 17:32
oscb pushed a commit that referenced this pull request Dec 15, 2023
## @segment/analytics-react-native [2.18.0-beta.1](https://github.com/segmentio/analytics-react-native/compare/@segment/analytics-react-native-v2.17.0...@segment/analytics-react-native-v2.18.0-beta.1) (2023-12-15)

### Features

* add support for hasUnmappedDestinations ([#905](#905)) ([545d596](545d596))
* consent plugin updates and test cases ([#894](#894)) ([ff1d332](ff1d332))
* RN 0.72 Upgrade ([03f13a1](03f13a1))

### Dependencies

* **@segment/sovran-react-native:** upgraded to 1.1.0-beta.1
oscb pushed a commit that referenced this pull request Dec 15, 2023
## @segment/analytics-react-native [2.18.0](https://github.com/segmentio/analytics-react-native/compare/@segment/analytics-react-native-v2.17.0...@segment/analytics-react-native-v2.18.0) (2023-12-15)

### Features

* add support for hasUnmappedDestinations ([#905](#905)) ([545d596](545d596))
* consent plugin updates and test cases ([#894](#894)) ([ff1d332](ff1d332))
* RN 0.72 Upgrade ([03f13a1](03f13a1))

### Dependencies

* **@segment/sovran-react-native:** upgraded to 1.1.0
oscb pushed a commit that referenced this pull request Dec 15, 2023
## @segment/analytics-react-native [2.18.0-beta.1](https://github.com/segmentio/analytics-react-native/compare/@segment/analytics-react-native-v2.17.0...@segment/analytics-react-native-v2.18.0-beta.1) (2023-12-15)

### Features

* add support for hasUnmappedDestinations ([#905](#905)) ([545d596](545d596))
* consent plugin updates and test cases ([#894](#894)) ([ff1d332](ff1d332))
* RN 0.72 Upgrade ([03f13a1](03f13a1))
oscb pushed a commit that referenced this pull request Dec 15, 2023
## @segment/analytics-react-native [2.18.0-beta.1](https://github.com/segmentio/analytics-react-native/compare/@segment/analytics-react-native-v2.17.0...@segment/analytics-react-native-v2.18.0-beta.1) (2023-12-15)

### Features

* add support for hasUnmappedDestinations ([#905](#905)) ([545d596](545d596))
* consent plugin updates and test cases ([#894](#894)) ([ff1d332](ff1d332))
* RN 0.72 Upgrade ([03f13a1](03f13a1))

### Dependencies

* **@segment/sovran-react-native:** upgraded to 1.1.0-beta.1
oscb pushed a commit that referenced this pull request Feb 1, 2024
## @segment/analytics-react-native [2.18.0](https://github.com/segmentio/analytics-react-native/compare/@segment/analytics-react-native-v2.17.0...@segment/analytics-react-native-v2.18.0) (2024-02-01)

### Features

* add support for hasUnmappedDestinations ([#905](#905)) ([545d596](545d596))
* consent plugin updates and test cases ([#894](#894)) ([ff1d332](ff1d332))
* RN 0.72 Upgrade ([03f13a1](03f13a1))

### Bug Fixes

* remove circular dependency in ConsentPlugin ([71e7eae](71e7eae))

### Dependencies

* **@segment/sovran-react-native:** upgraded to 1.1.0
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