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 logging for settings update #2684

Merged
merged 8 commits into from Jun 3, 2019
Merged

Conversation

alexsanford
Copy link
Contributor

@alexsanford alexsanford commented May 14, 2019

Closes #2645

See the issue for the event description. Note that the view property names have been changed to match the section slugs, and the values in settings are comma-separated.

This PR also adds a helper class for testing logged events.

Testing Instructions

  • Change some Sensei settings in a few different sections and save.
  • Ensure that one event is logged for each section with changed settings.
  • Ensure that the view and settings properties have the correct values based on the settings that were changed.

@alexsanford alexsanford added this to the 2.1.0 milestone May 14, 2019
@alexsanford alexsanford self-assigned this May 14, 2019
@alexsanford alexsanford changed the base branch from add/initial-event-tracking to master May 15, 2019 18:47
@alexsanford alexsanford marked this pull request as ready for review May 15, 2019 18:47
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Few super minor issues, otherwise looks good and works well.

@alexsanford
Copy link
Contributor Author

Thanks, @jom! Seems the CI stuff didn't run, which would have caught those linter issues. I got it all to be green now 👍

@donnapep Could you take a look at this one before we merge?

@donnapep
Copy link
Collaborator

When changing the checkbox settings under Email Notifications, I only get 3 distinct settings being logged - email_learners, email_teachers, email_global. There's no way to differentiate between the different settings within each of those groups.

@alexsanford alexsanford requested a review from jom May 27, 2019 17:59
@alexsanford
Copy link
Contributor Author

@donnapep Good catch on that one. This proved to be a bit tricky. In every other case when a setting changes, we want only to log the setting field name. However, for the checkboxes, the setting is stored as an array of strings, where each string is the name of the individual checkbox that was checked.

I added some code to handle this case (along with tests). It seems to work well, but please do re-test.

Also @jom I've re-requested your review because I would like you to take a look at the new code if you have a chance (944c500). It is functional, but I'm a bit worried about the readability. Would appreciate your input 🙂

@donnapep
Copy link
Collaborator

donnapep commented May 28, 2019

@alexsanford Thanks! This is what I'm seeing logged now:

Screen Shot 2019-05-28 at 7 01 30 AM

Is it possible to drop the email_learners, email_teachers and email_global arrays and just log the key (e.g. learner-graded-quiz, teacher-quiz-submitted etc.)? There are also a few extraneous empty arrays being logged that don't map to a setting (email_learners[], email_teachers[], email_global[]).

@alexsanford
Copy link
Contributor Author

@donnapep This should be fixed now 👍

Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

All good now!

@alexsanford alexsanford merged commit 810e5f0 into master Jun 3, 2019
@alexsanford alexsanford deleted the add/logging-settings-save branch June 3, 2019 13:15
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.

Log when the settings are updated
3 participants