Track used grace credits (Based on issue #2118) #7226

hemmatio · 2024-09-24T16:19:19Z

Proposed Changes

This pull request is based on issue #2118. Currently, there is no accessible way for students to view their grace credit usage, especially on a per-assignment basis. The issue outlined the creation of a new column on the student's home page, but the proposed solution is to add the used grace credits beside the assignment due date, if any.

Screenshots of your changes (if applicable)

UPDATE 02/10/24: Grace credits are on a newline, and the number of credits used is bolded.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)	X
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

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:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)
24/09/24:

Still need to create test cases for this change before requesting a review. ✅
Additionally, unsure about the location of the changes. Seems quite unorganized to add the changes to app/views/assignments/_assignment_date.html.erb, however this was done as the div used in _assignment_date.html.erb seems to force any other text onto another line, rather than remaining inline.

25/09/24:

Do I need to update the project documentation? This change isn't really a feature... is it? ✅
Still unsure about location of HTML change. ✅

02/10/24:

Updated grace_credits_used_for method.
Moved string into a locale file.
Bolded grace credits used, also on a new line.
Updated test cases to mimic behaviour found in grace_period_submission_rule.rb, now works for groupings.

…sage to _assignment_date.html.erb view

… track-used-grace-credits

hemmatio · 2024-09-26T16:11:58Z

spec/models/student_spec.rb

+      end
+
+      it 'updates remaining grace credits after deductions' do
+        create(:grace_period_deduction, membership: @membership, deduction: 2)


I don't like how I run create(:grace_period_deduction, membership: @membership, deduction: 2) two times. Putting this line in a before-do block causes the other tests in the context block to be affected, however.

Would it be more optimal to create a new context block for these two test cases?

Yes, a context block here is good. Something like context 'where there is a deduction'

david-yz-liu

@hemmatio good work. I left a few inline comments; in response to your questions:

I'd actually like it if the "grace credits used" text were on a separate paragraph (<p>) underneath the due date. So yeah, not in the _assignment_date.html.erb partial.
It's okay not to include a documentation change for this PR.

david-yz-liu · 2024-09-27T15:13:39Z

app/models/student.rb

@@ -250,6 +250,14 @@ def released_result_for?(assessment)
    end
  end

+  def grace_credits_used_for(assessment)
+    grouping = accepted_groupings.find_by(assessment_id: assessment.id)  # Find the grouping for this assessment


The querying here is good, but you can make better use of existing methods, in particular accepted_grouping_for.

There may also be a Grouping instance method that you can use to get the number of grace credits used by the grouping. Note that in a grouping, all members have the same grace credit deductions.

david-yz-liu · 2024-09-27T15:16:12Z

app/views/assignments/_assignment_date.html.erb

@@ -18,4 +18,8 @@
      (<%= distance_of_time_in_words_to_now(due_date) %>)
    <% end %>
  <% end %>
+  <% grace_credits_used = @current_role.grace_credits_used_for(assignment) %>
+  <% if grace_credits_used > 0 %>
+    <span> - Grace Credits Used: <%= grace_credits_used %></span>


Move the string into a locale file (look at how other user-facing strings are defined in this file)

Also, let's bold the number of grace credits used.

david-yz-liu · 2024-09-27T15:17:13Z

spec/models/student_spec.rb

+      end
+
+      it 'updates remaining grace credits after deductions' do
+        create(:grace_period_deduction, membership: @membership, deduction: 2)


Yes, a context block here is good. Something like context 'where there is a deduction'

… track-used-grace-credits

hemmatio · 2024-10-02T02:22:30Z

spec/models/student_spec.rb

+        expect(@student1.remaining_grace_credits).to eq(3)
+      end
+
+      # it 'deducts grace credits from each group member' do  # How to make this testcase..?


I'm attempting to create a grace credit deduction for a group assignment, and then check that each member in the group has received the deduction. I simulated this on my own, and it works as expected, but I want to be able to explicitly test for this with RSpec.
I'm unable to recreate this scenario within RSpec, as the grace credit deductions are applied to each membership, rather than a group. There may be another method to apply grace credit deductions on a group basis, but I have not found that method. I tried to investigate how the grace credit deductions are applied to a group, but I'm not able to trace it through fully.
I found that the SubmissionsJob method perform in app/jobs/submissions_job.rb runs when an instructor collects assignments and applies the grace deduction. I was hoping to find some sort of for-loop that applies the grace credit deduction equally to each member within a grouping so that I could mimic that behaviour within my test case.

hemmatio · 2024-10-02T02:23:10Z

Changelog.md

@@ -6,6 +6,11 @@

 ### ✨ New features and improvements

+- Add visual indicator on a per-assignment basis for used grace credits (#7226)
+- Improve textviewer rendering speed (#7211)


Lines 10 through 13 are from a merge

This merge is incorrect. These lines were moved under v2.5.2, and should be removed from the unreleased section.

In general, there shouldn't be any "lines from a merge". If there is code that appears in a diff on GitHub that you didn't mean to include yourself, that means that there was an error in the merge process that should be resolved.

coveralls · 2024-10-02T15:51:13Z

Pull Request Test Coverage Report for Build 11156833777

Details

21 of 21 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.004%) to 91.596%

Totals
Change from base Build 11155066813:	0.004%
Covered Lines:	40513
Relevant Lines:	43558

💛 - Coveralls

david-yz-liu · 2024-10-03T01:22:10Z

Changelog.md

@@ -6,6 +6,11 @@

 ### ✨ New features and improvements

+- Add visual indicator on a per-assignment basis for used grace credits (#7226)
+- Improve textviewer rendering speed (#7211)


This merge is incorrect. These lines were moved under v2.5.2, and should be removed from the unreleased section.

In general, there shouldn't be any "lines from a merge". If there is code that appears in a diff on GitHub that you didn't mean to include yourself, that means that there was an error in the merge process that should be resolved.

david-yz-liu · 2024-10-03T01:28:19Z

app/views/assignments/_row.html.erb

 <tr>
  <td>
    <%= link_to assignment_text, route %>
  </td>
  <td>
    <%= render partial: 'assignments/assignment_date',
               locals: { assignment: assignment } %>
+    <% if grace_credits_used > 0 %>
+      <span><%= I18n.t('assignments.grace_credits_used', grace_credits_used: grace_credits_used).html_safe%></span>


It's a good instinct to use html_safe here, but there's a more idiomatic Rails way to achieve this. See https://guides.rubyonrails.org/i18n.html#using-safe-html-translations

This is the kind of insight and maturity that I hope to develop one day! I initially used I18n.t because I saw it being used within _assignment_date.html.erb. When the HTML tags were being escaped, I looked on StackOverflow for an I18n-specific solution! It never came to my mind that I might be overthinking things 😪

@hemmatio oh honestly I'm not sure it was "insight" or "maturity", I just happened to know about this because of experience working with Rails, and I suppose reading through the guide. There's a lot of great info in these guides!

…ace_credits_used locale tag to be more consistent in HTML safety

… track-used-grace-credits

hemmatio added 4 commits September 24, 2024 12:02

Created grace_credits_used_for() method in student class, and added u…

a4a84d7

…sage to _assignment_date.html.erb view

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

b9c756a

… track-used-grace-credits

updated alignment of multi-line spanning line

0c4962e

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

42fca25

… track-used-grace-credits

hemmatio marked this pull request as ready for review September 26, 2024 03:20

updated changelog.md

bbcfb1f

hemmatio requested a review from david-yz-liu September 26, 2024 16:06

hemmatio commented Sep 26, 2024

View reviewed changes

david-yz-liu reviewed Sep 27, 2024

View reviewed changes

hemmatio marked this pull request as draft October 2, 2024 02:01

hemmatio added 2 commits October 1, 2024 22:11

rubocop issues

a1166cc

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

e436ea1

… track-used-grace-credits

hemmatio commented Oct 2, 2024

View reviewed changes

updated testcases to be compatible with groupings

22767f0

hemmatio marked this pull request as ready for review October 2, 2024 14:35

ran i18n-tasks normalize to normalize my added i18n key

37bef6b

hemmatio requested a review from david-yz-liu October 2, 2024 15:57

david-yz-liu reviewed Oct 3, 2024

View reviewed changes

hemmatio added 3 commits October 3, 2024 01:37

Fixed incorrect merge conflicts (from previous commit), refactored gr…

11e9661

…ace_credits_used locale tag to be more consistent in HTML safety

Merge branch 'master' of https://github.com/MarkUsProject/Markus into…

d6d554c

… track-used-grace-credits

retrigger tests

6ba132f

david-yz-liu approved these changes Oct 3, 2024

View reviewed changes

david-yz-liu merged commit 5849875 into MarkUsProject:master Oct 3, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track used grace credits (Based on issue #2118) #7226

Track used grace credits (Based on issue #2118) #7226

hemmatio commented Sep 24, 2024 •

edited

Loading

hemmatio Sep 26, 2024

david-yz-liu Sep 27, 2024

david-yz-liu left a comment

david-yz-liu Sep 27, 2024

david-yz-liu Sep 27, 2024

david-yz-liu Sep 27, 2024

david-yz-liu Sep 27, 2024

hemmatio Oct 2, 2024

hemmatio Oct 2, 2024

david-yz-liu Oct 3, 2024

coveralls commented Oct 2, 2024 •

edited

Loading

david-yz-liu Oct 3, 2024

david-yz-liu Oct 3, 2024

hemmatio Oct 3, 2024

david-yz-liu Oct 3, 2024

Track used grace credits (Based on issue #2118) #7226

Track used grace credits (Based on issue #2118) #7226

Conversation

hemmatio commented Sep 24, 2024 • edited Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-yz-liu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 2, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11156833777

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemmatio commented Sep 24, 2024 •

edited

Loading

coveralls commented Oct 2, 2024 •

edited

Loading