[Amplitude] Implement section setup complete/cancelled #49375
Conversation
rshipp
changed the title
rshipp
force-pushed
the
amplitude-section-completed
branch
from
dcd65f6
to
f1ce490
Compare
…, and format userId based on environment
rshipp
force-pushed
the
amplitude-section-completed
branch
from
10afc1f
to
46762f1
Compare
| @@ -406,4 +408,78 @@ describe('EditSectionForm', () => { | |||
| const lessonExtrasField = wrapper.find('LessonExtrasField'); | |||
| assert.equal(lessonExtrasField.length, 1); | |||
| }); | |||
|
|
|||
| it('sends completed event when save is clicked', () => { | |||
| const wrapper = mount( | |||
There was a problem hiding this comment.
Do you need mount here to ensure the button calls the appropriate function?
There was a problem hiding this comment.
I don't actually know what mount does, I assumed from its usage elsewhere that's just how you instantiate React components. is there a different way that'd be better here?
There was a problem hiding this comment.
You can also use shallow, which renders just the top level component, whereas mount renders all the child component as well. We generally prefer using shallow over mount but there are cases where mount is necessary. I'm not 100% sure but I think this might be one of those cases
There was a problem hiding this comment.
The tests still pass with shallow, so that seems good!
| const {section, courseOfferings, isNewSection} = this.props; | ||
|
|
||
| const courseName = courseOfferings.hasOwnProperty(section.courseOfferingId) | ||
| ? courseOfferings[section.courseOfferingId].display_name |
There was a problem hiding this comment.
Just checking that we want the display_name here (which will change based on locale) and not the id?
(I added this comment below then realized it belonged here -- sorry for the duplicate)
There was a problem hiding this comment.
Updated based on slack discussion in e05f83c.
Amplitude instrumentation for
Section Setup CompletedandSection Setup Cancelledevents. Based on #49374.When reviewing, please nitpick as much as you want, since this may be used as example code for other teams for later Amplitude instrumentation.
Links
Testing story
New unit tests, and manual testing locally.