-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#8607] Student: edit submission: show evaluees in alphabetical order #11482
[#8607] Student: edit submission: show evaluees in alphabetical order #11482
Conversation
|
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
1563038
to
e9572a5
Compare
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
It would be good if you can add tests for it, and yes it should be in question-submission-form.component.spec.ts |
Hi @ypinhsuan, my comment was hidden in the review page but Should the sort go into a method? Was thinking of putting it as a component class method but felt that it should be private as it is merely a class helper function, although javascript does not seem to have such a concept, so I'm not sure how to abstract it out. I'm also trying to implement this idea, as I would be able to just test this method instead of creating the whole component which I think would require a stub for QuestionSubmissionFormModel to instantiate the component and it seems complicated as the model is quite big. Any advice is appreciated. I'll be working on E2E tests in the meantime as I think I know how it should be done. |
Hi! Sorry for missing your previous comment, I've replied in that thread.
For front-end component tests, you can create a QuestionSubmissionModel object for testing. Perhaps looking at how
Nope. Emails for E2E testing are fake emails. |
@ypinhsuan Thanks for pointing me to the right places! I'm currently working on the E2E tests in I've got a few questions on my approach.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frontend component tests look great!
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
It seems like adding E2E test requires quite a bit of work (adding a new numscale question with multiple response and creating methods in For debugging E2E tests:
This part points you to the part which causes test failure. And the assertion error shows the actual vs expected output. In this case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. For your review @jianhandev @daongochieu2810
Guys, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢 |
For your kind review @jianhandev @daongochieu2810 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just some minor issues I've picked up:
src/web/app/components/question-submission-form/question-submission-form.component.spec.ts
Outdated
Show resolved
Hide resolved
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
src/web/app/components/question-submission-form/question-submission-form.component.spec.ts
Outdated
Show resolved
Hide resolved
src/web/app/components/question-submission-form/question-submission-form.component.spec.ts
Outdated
Show resolved
Hide resolved
Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
Fixes #8607
Sort the list of FeedbackResponseRecipient according to their name, using the recipientList - which is sorted within the component - as a reference. This will fix ensure that names of evaluees for number scale questions will not be shown according to their email address.
Example
Current implementation (sort by email)
Screen.Recording.2022-01-15.at.8.07.52.PM.mov
New implementation from PR (sort by name)
Screen.Recording.2022-01-15.at.8.02.00.PM.mov