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: anonymous props that's not moved to sensitive props #12931

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Jan 10, 2025

Description

Add a legacy compatible process in the addProperties function to handle the object props that contains a anonymous: true and move them to sensitiveProperties instead.

It was planned that code would use addSensistive properties, but sometimes, we get some mixed props from places like a getAnalyticsParams function that returns a lot props and passes them along far before the event is built.
It would require a too big refactoring to split these props at the source.
I decided to add a new processing in the addProperties directly to filter anonymous props passed as normal props and redirect them.

Even if this new additional processing is executed for all the addProperties calls, it is not adding enough processing time to negatively impact performances.

Related issues

Fixes #12728

Manual testing steps

  1. View the tx approval screen
  2. check Approval Started event props
  3. generates both a non-anonymous and an anonymous event.
  4. Only anonymous event contains number_tokens_requested

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@NicolasMassart NicolasMassart self-assigned this Jan 10, 2025
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@NicolasMassart NicolasMassart marked this pull request as ready for review January 10, 2025 18:07
@NicolasMassart NicolasMassart requested a review from a team as a code owner January 10, 2025 18:07
@NicolasMassart NicolasMassart added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 10, 2025
@NicolasMassart NicolasMassart added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 14, 2025
Copy link
Contributor

github-actions bot commented Jan 14, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 62a620d
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4ffb17ac-d155-43d1-ac3d-5ba01cd47b15

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

[Bug]: getAnalyticsParams return anonymous props that's not moved to sensitive props
2 participants