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.

Sign up for GitHub

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

Jump to bottom

Add amplitude event for moving rubric window #57931

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Add amplitude event for moving rubric window #57931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19bce8a
Added amplitude event for moving rubric window
Nokondi Apr 10, 2024
0c65efd
test fix
Nokondi Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/src/lib/util/AnalyticsConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ const EVENTS = {
TA_RUBRIC_STUDENT_AI_SUBMITTED: 'TA Rubric Student AI Level Submitted',
TA_RUBRIC_AI_EVAL_FROM_SECTION:
'TA Rubric AI Eval started from section request',
TA_RUBRIC_WINDOW_MOVE_START: 'TA Rubric window move start',
TA_RUBRIC_WINDOW_MOVE_END: 'TA Rubric window move end',

// AI Tutor
AI_TUTOR_PANEL_OPENED: 'AI Tutor Panel Opened',
Expand Down
16 changes: 16 additions & 0 deletions apps/src/templates/rubrics/RubricContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import Draggable from 'react-draggable';
import {TAB_NAMES} from './rubricHelpers';
import aiBotOutlineIcon from '@cdo/static/ai-bot-outline.png';
import HttpClient from '@cdo/apps/util/HttpClient';
import analyticsReporter from '@cdo/apps/lib/util/AnalyticsReporter';
import {EVENTS} from '@cdo/apps/lib/util/AnalyticsConstants';

// product Tour
import './introjs.scss';
Expand Down Expand Up @@ -108,6 +110,19 @@ export default function RubricContainer({
const onStopHandler = (event, dragElement) => {
setPositionX(dragElement.x);
setPositionY(dragElement.y);
analyticsReporter.sendEvent(EVENTS.TA_RUBRIC_WINDOW_MOVE_END, {
...(reportingData || {}),
window_x_end: dragElement.x,
window_y_end: dragElement.y,
});
};

const onStartHandler = (event, dragElement) => {
analyticsReporter.sendEvent(EVENTS.TA_RUBRIC_WINDOW_MOVE_START, {
...(reportingData || {}),
window_x_start: dragElement.x,
window_y_start: dragElement.y,
});
};

// Currently the settings tab only provides a way to manually run AI.
Expand Down Expand Up @@ -175,6 +190,7 @@ export default function RubricContainer({
return (
<Draggable
defaultPosition={{x: positionX, y: positionY}}
onStart={onStartHandler}
onStop={onStopHandler}
>
<div
Expand Down
43 changes: 43 additions & 0 deletions apps/test/unit/templates/rubrics/RubricContainerTest.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,4 +762,47 @@ describe('RubricContainer', () => {
expect(queryByText('Getting Started with AI Teaching Assistant')).to.not
.exist;
});

it('sends event when window is dragged', async function () {
const sendEventSpy = sinon.spy(analyticsReporter, 'sendEvent');
stubFetchEvalStatusForUser(readyJson);
stubFetchEvalStatusForAll(readyJsonAll);
stubFetchAiEvaluations(mockAiEvaluations);
stubFetchTeacherEvaluations(noEvals);
stubFetchTourStatus({seen: true});

const {getByTestId} = render(
<Provider store={store}>
<RubricContainer
rubric={defaultRubric}
studentLevelInfo={defaultStudentInfo}
teacherHasEnabledAi={true}
currentLevelName={'test_level'}
reportingData={{}}
open
/>
</Provider>
);

await wait();

const element = getByTestId('draggable-test-id');

// simulate dragging
fireEvent.mouseDown(element, {clientX: 0, clientY: 0});
fireEvent.mouseMove(element, {clientX: 100, clientY: 100});

expect(sendEventSpy).to.have.been.calledWith(
EVENTS.TA_RUBRIC_WINDOW_MOVE_START,
{window_x_start: 0, window_y_start: 0}
);

fireEvent.mouseUp(element);

expect(sendEventSpy).to.have.been.calledWith(
EVENTS.TA_RUBRIC_WINDOW_MOVE_END,
{window_x_end: 0, window_y_end: 0}
Comment on lines +803 to +804
Copy link
Member

Choose a reason for hiding this comment

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

this is confusing... shouldn't these be 100/100? are you able to validate that the right values come through in manual testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works locally. This is something I read about with the way that react testing library renders components. I had a similar issue when writing the unit test for moving the window (i.e., it didn't return the correct values, but it did register that the values had changed). I'll see if I can find a link to what I read before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, meant to say that I've tested manually and gotten the correct results:
image

Copy link
Member

Choose a reason for hiding this comment

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

that would be great. I'm not sure how common this will be, but until it becomes common knowledge I would suggest including a short explanation with a link to the issue as a code comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the explanation that I pieced together: React Testing Library uses JSDom as the basis for its render function. JSDom doesn't actually render elements, so everything will show up with a position of (0, 0). For the draggable unit test, I was able to get around this because the draggable component applies a transformation to the component, so if you look at the translation, you'll see the change, but the raw x and y position values will always show as (0, 0). It's possible you can get more accurate position readings by using RTL with Jest, but I haven't confirmed that.
image
jsdom/jsdom#1590

);
sendEventSpy.restore();
});
});