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.

Sign up for GitHub

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

Jump to bottom

E2157. Fix issues related to deadlines and late policies #2127

Merged
merged 18 commits into from
Feb 1, 2022
Merged

E2157. Fix issues related to deadlines and late policies #2127

merged 18 commits into from
Feb 1, 2022

Conversation

wshubham
Copy link
Contributor

@wshubham wshubham commented Nov 1, 2021

Please set the title of your pull request in the following format: [Project id/Independent study/Issue fix/...]. project name, eg., E904. Integrating courses and assignments on Expertiza with Moodle

We suggest you including the below information in your pull request:

  • A description of the changes proposed in the pull request.
  • @mentions of the person or team responsible for reviewing proposed changes.

About Expertiza Bot

  • If you have any questions about comments given by the expertiza-bot (example), you could create a comment in your pull request with the format /dispute [UUID1] [UUID2] (example) then the professor and TAs will be notified.
  • Here is a video about how to communicate to expertiza-bot.

@expertiza-bot
Copy link
Contributor

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

This repository is being automatically checked for code-quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

Also, please spend some time looking at the instructions at the top of your course project writeup.
If you have any questions, please send email to expertiza-support@lists.ncsu.edu.
UUID 2 Warnings
61df ⚠️ Your pull request is less than 50 LoC.
If you are finished refactoring the code, please consider writing corresponding tests.
ecaa ⚠️ There are code changes, but no corresponding tests.
Please include tests if this PR introduces any modifications in behavior.

Generated by expertiza-bot

@expertiza-bot
Copy link
Contributor

expertiza-bot commented Nov 1, 2021 •
edited

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

This repository is being automatically checked for code-quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

Also, please spend some time looking at the instructions at the top of your course project writeup.
If you have any questions, please send email to expertiza-support@lists.ncsu.edu.
UUID 6 Warnings
e222 ⚠️ Your pull request is more than 500 LoC.
Please make sure you did not commit unnecessary changes, such as schema.rb, node_modules, change logs.
be55 ⚠️ There are one or more skipped/pending/focused test cases in your pull request. Please fix them.
c951 ⚠️ You should commit changes to the DB schema (db/schema.rb) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes.
c989 ⚠️ You should not change rails_helper.rb or spec_helper.rb file; please revert these changes.
3ecd ⚠️ You modified spec/factories/ folder; please double-check whether it is necessary.
35e1 ⚠️ There are many wildcard argument matchers (e.g., anything, any_args) in your tests.
To avoid shallow tests – tests concentrating on irrelevant, unlikely-to-fail conditions – please avoid wildcard matchers.
61df ✅ Your pull request is less than 50 LoC.
If you are finished refactoring the code, please consider writing corresponding tests.
ecaa ✅ There are code changes, but no corresponding tests.
Please include tests if this PR introduces any modifications in behavior.

Generated by expertiza-bot

@coveralls
Copy link

Coverage Status

Coverage decreased (-18.6%) to 38.15% when pulling 7e106b7 on wshubham:beta into 6ad3607 on expertiza:beta.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-18.6%) to 38.15% when pulling 7e106b7 on wshubham:beta into 6ad3607 on expertiza:beta.

@coveralls
Copy link

coveralls commented Nov 1, 2021 •
edited

Coverage Status

Coverage increased (+12.09%) to 56.441% when pulling 47fe4ec on wshubham:beta into 9d32a60 on expertiza:beta.

@expertiza-travisci-bot
Copy link

Hey @wshubham,
Your changes look good to me! 🎉

@nnhimes
Copy link
Collaborator

nnhimes commented Jan 23, 2022

https://expertiza.csc.ncsu.edu/index.php/E2157._Fix_issues_related_to_deadlines_and_late_policies

johnbumgardner
johnbumgardner requested changes Jan 26, 2022
View reviewed changes
Copy link
Collaborator

@johnbumgardner johnbumgardner left a comment

Choose a reason for hiding this comment

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

there was an empty file that i couldnt comment that seems confusing.

made a few other notes

app/controllers/late_policies_controller.rb Outdated
@@ -10,7 +11,7 @@ def action_allowed?
current_user.instructor_id == instructor_id
end
end

# display all late policies
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it necessary to have these comments on basic CRUD functions? i think any basic Rails dev should understand what new and edit does

app/models/late_policy.rb
@@ -11,7 +11,7 @@ class LatePolicy < ActiveRecord::Base
validates :penalty_unit, presence: true

validates :max_penalty, numericality: {greater_than: 0}
validates :max_penalty, numericality: {less_than: 50}
validates :max_penalty, numericality: {less_than: 100}
Copy link
Collaborator

Choose a reason for hiding this comment

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

write a test in late_policy_spec.rb for these validations

app/models/late_policy.rb
# whenever a policy is updated, all the existing penalty objects needs to be updated according to new policy
def self.update_calculated_penalty_objects(penalty_policy)
def self.update_calculated_penalty_objects(late_policy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it really necessary to use a class method here? we keep using them throughout the project and need to periodically refactor away from them.

app/views/assignments/edit/_due_dates.html.erb Outdated
@@ -502,19 +504,20 @@

<input name="metareviewAllowed" type="hidden" value="false"/>
<%= check_box_tag('metareviewAllowed', 'true', @metareview_allowed, :onclick => "removeOrAddMetareview()") %>
<%= label_tag('metareviewAllowed', 'Use meta-review deadline') %><br/>
<%= label_tag('metareviewAllowed', 'Use meta-review deadline') %><br/><br/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be in snake case not camel case

johnbumgardner
johnbumgardner approved these changes Feb 1, 2022
View reviewed changes
Copy link
Collaborator

@johnbumgardner johnbumgardner left a comment

Choose a reason for hiding this comment

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

LGTM

@nnhimes nnhimes merged commit 1495725 into expertiza:beta Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnbumgardner johnbumgardner johnbumgardner approved these changes

Assignees

@nnhimes nnhimes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@wshubham @expertiza-bot @coveralls @expertiza-travisci-bot @nnhimes @johnbumgardner @alunavat