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

Naif/add-default-token-login-tracking. #671

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

naif-deriv
Copy link
Contributor

Clickup link:
Fixes issue: #

This PR contains the following changes:

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Developers Note (Optional)

Pre-launch Checklist (For PR creator)

As a creator of this PR:

  • ✍️ I have included clickup id and package/app_name in the PR title.
  • 👁️ I have gone through the code and removed any temporary changes (commented lines, prints, debug statements etc.).
  • ⚒️ I have fixed any errors/warnings shown by the analyzer/linter.
  • 📝 I have added documentation, comments and logging wherever required.
  • 🧪 I have added necessary tests for these changes.
  • 🔎 I have ensured all existing tests are passing.

Reviewers

Pre-launch Checklist (For Reviewers)

As a reviewer I ensure that:

  • ✴️ This PR follows the standard PR template.
  • 🪩 The information in this PR properly reflects the code changes.
  • 🧪 All the necessary tests for this PR's are passing.

Pre-launch Checklist (For QA)

  • 👌 It passes the acceptance criteria.

Pre-launch Checklist (For Maintainer)

  • [MAINTAINER_NAME] I make sure this PR fulfills its purpose.

@naif-deriv naif-deriv changed the title add default token login tracking. Naif/add-default-token-login-tracking. Jul 3, 2024
'event': 'login_flow_error',
'properties': <String, dynamic>{
'app_id': appId,
'login_provider': 'deafult_token',
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this mean ? and it has a typo. default not deafult

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be used when the user is automatically logged in using the saved token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, what it is considered as error ? login_flow_error

Copy link
Contributor Author

@naif-deriv naif-deriv Jul 4, 2024

Choose a reason for hiding this comment

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

  • The thing is, I faced this issue when the default token belonged to an account to qa10 before it was rebuilt, after rebuilding, the app would automatically log in using the token of a non-existent account, and it would throw an error.

  • When RudderStack tries to handle this error, it refuses to do so because of missing params, login_provider which is what type of log in was initiated, and that default token type was not being handled.

  • Theoretically, this never happens on production, but still, developers will face it at least once every qa rebuild, and it makes sense to handle it.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the explanation. Understood. Looks good to me. Just ensure this is clear to Reeja and the people who would be analysing the data.

@abedelaziz-deriv
Copy link
Contributor

please fix the title and update the PR. We should be able to marge aftewards.

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