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

refactor: log event name change #550

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

refactor: log event name change #550

wants to merge 3 commits into from

Conversation

Sanskar2001
Copy link
Contributor

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Renamed ORCA_ELEMENTS_CALLED to WEB_SDK_IFRAME_MOUNTED

How did you test it?

Checklist

  • I ran npm run re:build
  • I reviewed submitted code
  • I added unit tests for my changes where possible

@Sanskar2001 Sanskar2001 added the DO NOT MERGE Use this label if you want your PR to restrict from merging label Aug 8, 2024
@Sanskar2001 Sanskar2001 removed the DO NOT MERGE Use this label if you want your PR to restrict from merging label Aug 21, 2024
@@ -39,7 +39,7 @@ type eventName =
| APP_REINITIATED
| LOG_INITIATED
| LOADER_CALLED
| ORCA_ELEMENTS_CALLED
| WEB_SDK_IFRAME_MOUNTED
Copy link
Contributor

@ArushKapoorJuspay ArushKapoorJuspay Aug 21, 2024

Choose a reason for hiding this comment

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

Could you please explain the use case of this event? Can it be something like HYPER_ELEMENTS_CALLED or ELEMENTS_CALLED?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wanted to make it more readable to merchant in control center's audit trail. The names like HYPER_ELEMENTS_CALLED or ELEMENTS_CALLED makes more sense to Hyperswitch devs, but we wanted to make it more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please rename this to PAYMENT_ELEMENTS_INITIATED. Since we should not have WEB as a prefix for any log event. Also since this is not the only iframe which gets mounted, it will be confusing moving forward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of mobile counter part of this event name is APP_RENDERED so we can use WEB as prefix for this one. Agree, PAYMENT_ELEMENTS_INITIATED is more a generic and umbrella term as it's not only about an iframe. But to a merchant iframe mounted is a more tangible event instead of something like elements initiated. The merchant should be able to derive some sense out of an event name. Let's decide a self explanatory event name for this before moving forward. cc @seekshiva @akash-c-k @PritishBudhiraja

@PritishBudhiraja PritishBudhiraja removed the request for review from vsrivatsa-edinburgh August 22, 2024 09:04
@PritishBudhiraja PritishBudhiraja removed the request for review from swamu October 29, 2024 08:46
@PritishBudhiraja PritishBudhiraja added Enhancement / Refactoring New feature or request or any refactoring Added Comments - waiting for author Add this label when comments added in the PR and waiting for the author to get those resolved. labels Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Comments - waiting for author Add this label when comments added in the PR and waiting for the author to get those resolved. Enhancement / Refactoring New feature or request or any refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants