render (hidden) label for assignments according to visibility setting (#7785)#7944
render (hidden) label for assignments according to visibility setting (#7785)#7944philipkukulak wants to merge 1 commit into
Conversation
philipkukulak
changed the title
Coverage Report for CI Build 25680949457Coverage increased (+0.01%) to 91.74%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
Nice work, @philipkukulak! I just left a few small comments.
| view_flow.content.delete(content_key) | ||
| end | ||
|
|
||
| def assignment_hidden_label?(assignment) |
There was a problem hiding this comment.
In general, only very broadly applicable code should be added to application_helper.rb.
This method (assignment_hidden_label?) can be moved into the Assessment model as an instance method; I would rename it it to currently_hidden?.
| false | ||
| end | ||
|
|
||
| def formatted_assignment_visibility_label(assignment, base_text) |
There was a problem hiding this comment.
Because this method is specifically tied to views, it's good to put it in a helper, but I would move it into a new assessments_helper.rb.
Note that the logic here doesn't just apply to assignments, but also to grade entry forms.
| <% title = "#{assessment.parent_assignment.short_identifier} #{PeerReview.model_name.human}" %> | ||
| <% end %> | ||
| <% if assessment.is_hidden %> | ||
| <% if assessment.is_a?(Assignment) %> |
There was a problem hiding this comment.
The class check here isn't necessary; we can apply the same logic to grade entry forms
|
|
||
| before do | ||
| travel_to(current_time) | ||
| create(:assignment, course: course, short_identifier: 'A_plain', description: 'Plain Visible') |
There was a problem hiding this comment.
You can create this assignment within the visibility_cases array above (include as a new key within each hash). I wouldn't recommend adding short_identifier and description fields, as these are extraneous to the test case.
Then further below, you can parameterize the test cases by calling it within a loop:
visibility_cases.each do |visibility_case|
it ... <label> ... do
get_as role, ...
...
end
endSimilar comment for the test cases below.
| - Fixed reserved interpolation key `%{format}` in `download_errors.unrecognized_format` locale string, renamed to `%{file_format}` (#7894) | ||
| - Fix version mismatch between container client and database server (#7916) | ||
| - Fixed filter Canvas Test Student from roster sync (#7926) | ||
| - Fixed `(hidden)` assignment labeling for assignments with `visible_on` and/or `visible_until` set (#7785) |
There was a problem hiding this comment.
"assignments" -> "assessments"
Also, the number in the changelog entry is actually the PR number, not the issue number.
3 participants
Proposed Changes
From an instructor/TA view, the visibility of an assignment is not correctly displayed if visible-from and/or visible-until settings are set because the labelling logic only covers the binary "visible or hidden" settings. The proposed changes correctly labels assignments based on whether or not they are visible to students in the dashboard view, the instructor/TA assignment list view, and the instructor/TA assignment list view dropdown menu.
Screenshots of your changes (if applicable)
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
n/a