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 analytics events #1036

Merged
merged 15 commits into from
Jun 21, 2017
Merged

Add analytics events #1036

merged 15 commits into from
Jun 21, 2017

Conversation

francocorreasosa
Copy link
Contributor

@francocorreasosa francocorreasosa commented Jun 15, 2017

Implemented:

  • Show Forgot Password (forgot_password ready)
  • Submit Login (signin submit)
  • Submit Register (register submit)
  • Submit Forgot Password (forgot_password submit)
  • Submit OAuth connection (federated login)

Note: federated login will bring an object with more specific info about the event.

{
  name: string, // connection name
  strategy: string, // example: 'facebook', 'twitter', etc...
  action: string // 'signin' or 'signup'
} 

Resolve #1028

@francocorreasosa francocorreasosa changed the title WIP: Add analytics events Add analytics events Jun 16, 2017
@francocorreasosa francocorreasosa changed the title Add analytics events Add analytics events #1028 Jun 16, 2017
@francocorreasosa francocorreasosa changed the title Add analytics events #1028 Add analytics events Jun 16, 2017
src/core.js Outdated
'signin submit',
'signup submit',
'forgot_password submit',
'last_login ready',
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

@francocorreasosa francocorreasosa Jun 19, 2017

Choose a reason for hiding this comment

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

this array is exposed via lock.validEvents. if we remove this, the analytics script may not work as expected. (we use this to check if an event is available or not)

src/core.js Outdated
'signup submit',
'forgot_password submit',
'last_login ready',
'mfa_login ready'
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

.vscode/tags Outdated
!_TAG_FILE_SORTED 1 /0=unsorted, 1=sorted, 2=foldcase/
!_TAG_PROGRAM_AUTHOR Darren Hiebert /dhiebert@users.sourceforge.net/
!_TAG_PROGRAM_NAME Exuberant Ctags //
!_TAG_PROGRAM_URL http://ctags.sourceforge.net /official site/
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

@@ -5,6 +5,7 @@ import { authWithUsername, hasScreen } from './index';
import { cancelResetPassword, resetPassword } from './actions';
import { renderPasswordResetConfirmation } from './password_reset_confirmation';
import * as i18n from '../../i18n';
import * as l from '../../core/index';
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

@@ -6,6 +6,16 @@ import { logIn } from '../../quick-auth/actions';
import { displayName, socialConnections, authButtonsTheme } from '../../connection/social/index';

export default class SocialButtonsPane extends React.Component {
handleAuthButtonClick(lock, provider, isSignUp) {
const prov = typeof provider !== 'object' ? provider.toJS() : provider;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all values of the connection? Can we just send what type of strategy it is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its technically possible to have multiple strategies of the same type though, right? Thats why we were thinking of getting the name as well as the strategy. We dont need all the properties though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you specify what properties to keep? I'll work on this in a few minutes

Copy link
Member

Choose a reason for hiding this comment

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

You can have multiples connection names per strategy for enterprise (for social it might not be that common) But with connection name and strategy will be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, thanks 👍

@@ -6,6 +6,16 @@ import { logIn } from '../../quick-auth/actions';
import { displayName, socialConnections, authButtonsTheme } from '../../connection/social/index';

export default class SocialButtonsPane extends React.Component {
handleAuthButtonClick(lock, provider, isSignUp) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this method outside the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean in the same file, out of the class?

@hzalaz
Copy link
Member

hzalaz commented Jun 19, 2017

Also @francocorreasosa @ntotten some tests that the event is correctly emitted with be good

@luisrudge
Copy link
Contributor

@francocorreasosa it'd be good to add an entry in the readme about the events as well

README.md Outdated
- `forgot_password submit`: emitted when the user clicks on the submit button of the "Forgot password" screen.
- `signin submit`: emitted when the user clicks on the submit button of the "Login" screen.
- `signup submit`: emitted when the user clicks on the submit button of the "Register" screen.
- `federated login`: emitted when the user clicks on a social connection button. Has the connection name and the strategy as arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luisrudge Is this fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Sign up" is better

README.md Outdated
- `forgot_password ready`: emitted when the "Forgot password" screen is shown.
- `forgot_password submit`: emitted when the user clicks on the submit button of the "Forgot password" screen.
- `signin submit`: emitted when the user clicks on the submit button of the "Login" screen.
- `signup submit`: emitted when the user clicks on the submit button of the "Register" screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

better to use "Signup" screen here

Copy link
Contributor

@ntotten ntotten 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 to me

@ntotten
Copy link
Contributor

ntotten commented Jun 21, 2017

@luisrudge @hzalaz Looks like all the feedback is resolved. Could we get this merged? :)

luisrudge
luisrudge previously approved these changes Jun 21, 2017
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.

None yet

4 participants