Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

telemetry in autofill #498

Merged
merged 3 commits into from
Mar 14, 2019
Merged

telemetry in autofill #498

merged 3 commits into from
Mar 14, 2019

Conversation

sashei
Copy link
Contributor

@sashei sashei commented Mar 12, 2019

Fixes #122

To Do

  • double check the original issue to confirm it is fully satisfied
  • make sure CI builds are passing (e.g.: fix lint and other errors)

@sashei sashei requested a review from a team as a code owner March 12, 2019 22:46
@ghost ghost assigned sashei Mar 12, 2019
@ghost ghost added the in progress label Mar 12, 2019
@sashei sashei changed the title add actions to autofillservice, include telemetrystore telemetry in autofill Mar 12, 2019
is AutofillAction.Authenticate -> builder.buildAuthenticationFillResponse(this).asOptional()
is AutofillAction.Cancel -> Optional(null)
is AutofillAction.Error -> {
callback.onFailure(getString(R.string.autofill_error_toast, it.error.localizedMessage))
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 moving this out of the subscribe

is DataStore.State.Errored -> AutofillAction.Error(state.error)
}
}
.subscribe(dispatcher::dispatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when we dispatch an Optional? (ex: AutofillAction.Cancel -> Optional(null))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of these are Optional, the type for the Observable is AutofillAction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh I see. It dispatches the action, then it goes down and does the Optional stuff. Got it 🥇

.map {
@Suppress("IMPLICIT_CAST_TO_ANY")
when (it) {
is AutofillAction.Complete -> builder.buildFilteredFillResponse(this, listOf(it.login)).asOptional()
Copy link
Contributor

Choose a reason for hiding this comment

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

The Context here (this) is now the AutofillAction instead of the DatastoreState. Are these two interchangeable to get the package name correctly in the builder?

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 is the instance of the AutofillService in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh ok. Misread that in AS :)

@sashei sashei merged commit 708f700 into master Mar 14, 2019
@sashei sashei deleted the 122-autofill-telemetry branch March 14, 2019 15:22
@ghost ghost removed the in progress label Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record autofill telemetry events
2 participants