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

[#12894] Per-recipient stats are calculated based on student name, not email #12981

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Hkovin
Copy link

@Hkovin Hkovin commented Apr 4, 2024

Fixes #12894

Outline of Solution

This PR is ready for review. We solved the issue by modifying msq-question-statistics.calculation.ts. Instead of using "response.recipient" to index into perRecipientResponse we used "response.recipientEmail." We also created another variable to store the recipient names in order to populate the recipient property of the perRecipientResponses object.

@cedricongjh
Copy link
Contributor

hi @Hkovin, thank you for the PR, do fix the failing tests before we proceed to review it, thank you!

@Hkovin Hkovin force-pushed the 12894-per-recipient-stats-calculated-by-name branch from 12d5614 to 24ffc2c Compare April 13, 2024 20:41
Copy link
Contributor

@mingyuanc mingyuanc left a comment

Choose a reason for hiding this comment

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

Hey, the component testing is still failing, do look into it. Feel free to ping me if you encounter any difficulty.

@Hkovin
Copy link
Author

Hkovin commented Apr 16, 2024

Currently, the component test "MsqQuestionStatisticsComponent" in the file "src/web/app/components/question-types/question-statistics/msq-question-statistics.component.spec.ts" is failing. This is due to the code changes I made, which index the recipients based on email rather than name - which was needed in order to uniquely identify students. Unfortunately, with the logic needed to resolve this issue, I won't be able to pass this test.

@cedricongjh
Copy link
Contributor

Currently, the component test "MsqQuestionStatisticsComponent" in the file "src/web/app/components/question-types/question-statistics/msq-question-statistics.component.spec.ts" is failing. This is due to the code changes I made, which index the recipients based on email rather than name - which was needed in order to uniquely identify students. Unfortunately, with the logic needed to resolve this issue, I won't be able to pass this test.

hi @Hkovin, since you changed the logic, the tests have to change accordingly to reflect the change in logic as well, do go ahead and update the tests

@jckras
Copy link

jckras commented Apr 22, 2024

Currently, the component test "MsqQuestionStatisticsComponent" in the file "src/web/app/components/question-types/question-statistics/msq-question-statistics.component.spec.ts" is failing. This is due to the code changes I made, which index the recipients based on email rather than name - which was needed in order to uniquely identify students. Unfortunately, with the logic needed to resolve this issue, I won't be able to pass this test.

hi @Hkovin, since you changed the logic, the tests have to change accordingly to reflect the change in logic as well, do go ahead and update the tests

Hi @cedricongjh, we did attempt to change the logic in the tests, however the CI build still runs on the old tests that use the old logic.

@cedricongjh
Copy link
Contributor

the CI will run the latest version of the tests, including those in this PR, it appears that there are not any tests in the PR, do remember to push them, thank you

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.

Per-recipient stats are calculated based on student name, not email
4 participants