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

Student: feedback submission: distribution points question: explain if equal points are not allowed #8817

Closed
damithc opened this issue May 7, 2018 · 28 comments · Fixed by #8836
Assignees
Labels
a-UIX User Interface, User eXperience, responsiveness help wanted Moderate difficulty, small localized change; suitable for novice contributors
Milestone

Comments

@damithc
Copy link
Contributor

damithc commented May 7, 2018

v6.5.1

Scenario:
A session has a 'distribute points' question. It requires all points to be different
image

Current: When a respondent is submitting responses, the submission is rejected if the student gives equal points to all recipients, which is the correct behavior. However, the question description doesn't mention that points need to be different. The restriction is revealed only when the student tries to submit.

Suggested: When the question is presented to the respondent, also explain that equal points are not acceptable. That way, the respondent doesn't have to discover the restriction by trial-and-error.

@damithc damithc added a-UIX User Interface, User eXperience, responsiveness help wanted Moderate difficulty, small localized change; suitable for novice contributors f-Submissions labels May 7, 2018
@jimmy51997
Copy link

@damithc ..... I would like to work on this issue.... anywhere I can get help from?

@sukanta-27
Copy link
Member

@jimmy51997 As you are a first time contributor, I would advise you to start by taking one of our d.FirstTimers issues to get familiar with the codebase and workflow. But if you strongly feel that you want to take this issue, then you can also do that.

@jimmy51997
Copy link

@sukanta-27 Every first timer issue seems already to be taken up....what should i do?? Say if i necessarily want to work on this issue, How to start?

@sukanta-27
Copy link
Member

@jimmy51997 Figuring out what to do is a part of the task.

I hope your work environment is set and you have run the app in dev server. If you haven't, do that first.
Then find out which files needs to be modified to solve this issue, modify them, update html documents with GodMode, run tests, make sure all tests are passing. Then make a PR against the TEAMMATES master branch for review.

@jimmy51997
Copy link

@sukanta-27 I have set up the environment.........Thanks for the input.

@sukanta-27
Copy link
Member

@jimmy51997

Every first timer issue seems already to be taken up

Also, #8589 doesn't seem to have an open PR, if you want you can take it.

@jimmy51997
Copy link

@sukanta-27 ....Are u sure it doesn't have an open PR?? If not how to know if it has an open PR or not? one more thing...does this project have a chat channel?

@crphang
Copy link
Contributor

crphang commented May 10, 2018

@jimmy51997. Yes, no open PR is opened for it. You can look at the issue to see if a referenced PR is created to address it. A PR that fixes the issue should have '#issue_id' in the PR title and description.

@jimmy51997
Copy link

@crphang thanks...I think i got it now.

@sukanta-27
Copy link
Member

sukanta-27 commented May 10, 2018

does this project have a chat channel?

@jimmy51997 For TEAMMATES, the issue tracker acts as a chat channel. If you have any problem or want to reach out to other developers (Like a help request, feature request etc), then you can open an issue for it. Do remember to follow the appropriate issue template while creating an issue.

@mg14777
Copy link
Contributor

mg14777 commented May 14, 2018

I would like to work on this issue

@mg14777
Copy link
Contributor

mg14777 commented May 15, 2018

@damithc Sir, is this what you had in mind ? I made use of the instructions parameter in FeedbackSubmissionEditQuestion and modify it when the checkbox you highlighted above is ticked.
Another thing worth mentioning is that once a user starts allocating points the status does mention that same number of points shouldn't be given multiple times.
distribute-unequal

@damithc
Copy link
Contributor Author

damithc commented May 15, 2018

hmm... this is a neat way to get the job done but I'm a bit reluctant to piggy-back of visibility information for two reasons:

  • It feels like a 'trick' or a 'clever shortcut'. It is better to avoid such tricks to maintain the simplicity of the code.
  • Some users may automatically disregard text in gray thinking that it is a visibility explanation.

From a UIX point of view, it feels like the new instruction belongs together with the existing instruction shown in blue in your screenshot.

@mg14777
Copy link
Contributor

mg14777 commented May 15, 2018

@damithc I have changed it. I agree the visibility is poor. I was trying to make minimal changes to code to accomplish this. My bad. I have clubbed the instruction with the status display. The initial view is shown below:
distribute-unequal-initial

On halfway allocation of points, I display an error if any two allocations are same.
distribute-unequal-halfway

In the end, if all points have been distributed, the error is still shown until all points are different.
distribute-unequal-after

@sukanta-27
Copy link
Member

@mg14777 Just a thought, maybe Error: is not a good word to use here? The red font-color is enough to grab the attention of the user IMO.

@mg14777
Copy link
Contributor

mg14777 commented May 15, 2018

@sukanta-27 Thanks for your suggestion. The reason why I put Error was to differentiate it from the first statement "All points distributed" since the second one is specifically an error not a generic statement. But maybe that's just me 😅 I can change that sure.

@sukanta-27
Copy link
Member

sukanta-27 commented May 15, 2018

@mg14777 I would suggest you to wait for Prof. @damithc's suggestion before making any changes. 😄

@damithc
Copy link
Contributor Author

damithc commented May 16, 2018

@mg14777 Looks like we have two independent conditions but treat both of them as one. e.g., Even when all points are distributed, All points distributed! appear in red. Is it possible to make the two operate independently?

Good point @sukanta-27 but I guess no harm using the word Error as it will make it even more clearer. Note that some users can be color blind.

@mg14777
Copy link
Contributor

mg14777 commented May 16, 2018

@damithc The way it currently works is that the font color of both statements is dependent on whether there is an error or not. Once the user fixes the error All points distributed! turns green:
distribute-unequal-after-complete
We could completely separate them by having two different <p> elements for both messages and applying different formatting to them. Currently they are part of a single <p> element and the formatting is overridden by the error statement

@damithc
Copy link
Contributor Author

damithc commented May 16, 2018

We could completely separate them by having two different <p> elements for both messages and applying different formatting to them.

Let's give it a try

@mg14777
Copy link
Contributor

mg14777 commented May 16, 2018

@damithc I have separated the conditions now. Error statements are displayed separately whenever needed.

If all points have been distributed but some are same we display a green and red flag separately
distribute-unequal-after

If some points are left to be distributed it is simply stated in blue
distribute-unequal-halfway

If there is an overallocation two separate error statements are displayed for clarity
distribute-unequal-overallocation

@damithc
Copy link
Contributor Author

damithc commented May 16, 2018

Nice work!
While we are touching this code, see if you can tweak these things too.

  • I think it will look nicer if the messages are left-aligned.
  • It may be easier for the user to understand if we said something like Expected total is 300 but your total is 302! Please adjust numbers to make the total 300

@mg14777
Copy link
Contributor

mg14777 commented May 16, 2018

@damithc Does this look intuitive
distribute-unequal-halfway-1
distribute-unequal-overallocation-1
distribute-unequal-after-complete-1

@damithc
Copy link
Contributor Author

damithc commented May 16, 2018

Looks better.
Alignment: indent a bit?

The current color choice is a bit fuzzy (e.g., total<400 is shown in blue). Shall we do the following?

The total points should add up to 400. and Multiple options should not be given the same number of points. can always be in blue and remain static as they are simply the instructions.

In addition, and based on the actual points, we can also show these:
Actual total is 400. (green)
Actual total is 396! Please adjust points to add up to 400. (red)
Actual total is 401! Please adjust points to add up to 400. (red)
Multiple options are given 22!. Please ensure that multiple options are not given the same number of points. (red)

@mg14777
Copy link
Contributor

mg14777 commented May 16, 2018

I agree with most of them.
I was thinking if instead of Actual total is 396! Please adjust points to add up to 400. we can instead state the pending amount (or extra in case of overallocation) like Actual total is 396! Please distribute the remaining 4 points. because we are already specifying the constraint/instruction in the beginning as you suggested.

Another thing to clarify, should I replace Error: with icons then ?

In the last statement, what should happen if there are multiple sets of repetitions ? For example, if 2 users have been allocated 22 and other 2 have been allocated 23. Should we display all such repetitions. Not sure if this is applicable here, but from a scalability perspective that might lead to a lot of repetitions to display?

@damithc
Copy link
Contributor Author

damithc commented May 16, 2018

I was thinking if instead of Actual total is 396! Please adjust points to add up to 400. we can instead state the pending amount (or extra in case of overallocation) like Actual total is 396! Please distribute the remaining 4 points. because we are already specifying the constraint/instruction in the beginning as you suggested.

Yes, that's a good idea.

Another thing to clarify, should I replace Error: with icons then ?

We can try that. Use the standard bootstrap glyphicons we use in other places.

In the last statement, what should happen if there are multiple sets of repetitions ? For example, if 2 users have been allocated 22 and other 2 have been allocated 23. Should we display all such repetitions. Not sure if this is applicable here, but from a scalability perspective that might lead to a lot of repetitions to display?

Perhaps we limit to first duplication of points we detected? We can phrase it like this Multiple options are given the same number of points! e.g., 22 Please ensure that multiple options are not given the same number of points.

@mg14777
Copy link
Contributor

mg14777 commented May 17, 2018

@damithc I have made the following changes

Instructions are listed in blue and other notifications/messages follow
distribute-unequal-halfway-2
distribute-unequal-after-1

Since currently the functionality is implemented commonly for both Distribute points among recipients and options type of questions, I have added this feature for among Options type of questions as well. This includes conditions where at least one of the options must be different etc. On a side note, it seems like the recipients and options are indented differently atm. Maybe this is something that can be fixed ?
distribute-options-allsame
distribute-options-overallocation

Also I was wondering if I can create a PR and commit some of this now.

@damithc
Copy link
Contributor Author

damithc commented May 17, 2018

Looks good. Yes, can start a PR to seek prelim comments about the implementation approach.
One thing though: as there are two instructions, we should also have two 'status' messages too? i.e., there should be a ✅ or ❌ for each ℹ️

wenmogu pushed a commit that referenced this issue Jun 20, 2018
…xplain if equal points are not allowed (#8836)

* Distribute points questions enhancements

* change html template id to match jquery selector name

* instructions replaced + labels realigned

* uneven distribution flag fix and among options enhancements

* remove markup from tag files

* update message logic for backward compatibility

* improve styling code and function names

* fix form submission error

* update HTML files using God Mode

* add browser tests and fix minor bugs

* Update html files for remaining browser tests: God Mode

* Update html files for FeedbackRubricQuestionUiTest.java

* Update html pages for InstructorFeedbackEditPageUiTest.java

* remove unused imports

* fix indentation and instructions when recipient is hidden

* add more browser tests

* fix style errors

* update html files for InstructorEditStudentFeedbackPageUiTest and FeedbackRankQuestionUiTest

* add comments to explain logic

* fix contrib padding and remove unnecessary comments

* update HTML files

* refresh repeated points buffer

* improve comment and fix more info indent

* update html files

* fix indentation and typos
@tshradheya tshradheya added the e.4 label Jun 21, 2018
@tshradheya tshradheya added this to the V6.7.0 milestone Jun 21, 2018
jacoblipech pushed a commit to jacoblipech/teammates that referenced this issue Jun 27, 2018
…estion: explain if equal points are not allowed (TEAMMATES#8836)

* Distribute points questions enhancements

* change html template id to match jquery selector name

* instructions replaced + labels realigned

* uneven distribution flag fix and among options enhancements

* remove markup from tag files

* update message logic for backward compatibility

* improve styling code and function names

* fix form submission error

* update HTML files using God Mode

* add browser tests and fix minor bugs

* Update html files for remaining browser tests: God Mode

* Update html files for FeedbackRubricQuestionUiTest.java

* Update html pages for InstructorFeedbackEditPageUiTest.java

* remove unused imports

* fix indentation and instructions when recipient is hidden

* add more browser tests

* fix style errors

* update html files for InstructorEditStudentFeedbackPageUiTest and FeedbackRankQuestionUiTest

* add comments to explain logic

* fix contrib padding and remove unnecessary comments

* update HTML files

* refresh repeated points buffer

* improve comment and fix more info indent

* update html files

* fix indentation and typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-UIX User Interface, User eXperience, responsiveness help wanted Moderate difficulty, small localized change; suitable for novice contributors
Projects
None yet
6 participants