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

E2104. Email notification to reviewers and instructors. #1913

Closed
wants to merge 14 commits into from
Closed

E2104. Email notification to reviewers and instructors. #1913

wants to merge 14 commits into from

Conversation

harshkachhadia
Copy link

A description of the changes proposed in the pull request.

This amendment to the project involves adding an asynchronous deadline reminder mailer to the application.

  • In the due_date.rb file, whenever a new due date is created or an existing due date is updated, the 'start_reminder' method will be fired which will eventually be added to the delayed_job queue.

  • This job will be executed at a preconfigured time before deadline, where it will fire the method 'reminder' which will be added to the delayed job queue by the handle_asynchronously method of gem 'delayed_job_active_record'.

  • Inside the reminder method, we will fetch three attributes - assignment_id, deadline_type, due_at. These three attributes will be used to decide the deadline type ( submission or review or teammate review ), fetch the participant email for that assignment, fetch the deadline threshold and at the end send the email reminder at a specified threshold time before the deadline which will contain all the details such as assignment names link to assignment and assignment type ( submission or review or teammate review).

Person or team responsible for reviewing proposed changes.
@efg , @Saurabh110

@expertiza-bot
Copy link
Contributor

expertiza-bot commented Mar 20, 2021

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 ✅ Yay.
c51d You are modifying Gemfile or Gemfile.lock, please double check whether it is necessary.
You are suppose to add a new gem only if you have a very good reason. Try to use existing gems instead.
Please revert changes to Gemfile.lock made by the IDE.
4f9c You are requiring rspec gem, fixture-related gem(s) or different helper methods in RSpec tests.
There have already been included, you do not need to require them again. Please remove them.
e29c One or more of your test expectations do not have matchers.
To avoid shallow tests – tests concentrating on irrelevant, unlikely-to-fail conditions – please include matchers, such as comparisons (e.g., equal(expected_value)), the status change of objects (e.g., change(object, :value).by(delta)), error handlings (e.g., raise_error("message")).
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-travisci-bot
Copy link

Hey @harshkachhadia,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

@@ -0,0 +1,28 @@
require 'test_helper'
Copy link
Member

Choose a reason for hiding this comment

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

Please write RSpec tests under the spec folder.

Copy link
Author

Choose a reason for hiding this comment

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

Done writing and refactoring the same. Thank you for guiding the team. We appreciate it.

@Winbobob
Copy link
Member

Hi team, please fix all the code climate issues.

@coveralls
Copy link

Coverage Status

Coverage decreased (-32.3%) to 8.275% when pulling 527f541 on harshkachhadia:master into 3c5e966 on expertiza:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-32.3%) to 8.275% when pulling 527f541 on harshkachhadia:master into 3c5e966 on expertiza:master.

@coveralls
Copy link

coveralls commented Mar 22, 2021

Coverage Status

Coverage decreased (-10.02%) to 30.54% when pulling 0eb1e78 on harshkachhadia:master into 3c5e966 on expertiza:master.

reminder
end

def reminder
Copy link

Choose a reason for hiding this comment

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

Method reminder has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

# content_type: "text/html",
to: defn[:to])
end

Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

return (self.due_at - days_before_deadline).to_datetime
end

handle_asynchronously :start_reminder, run_at: proc { |i| i.when_to_run_start_reminder }
Copy link

Choose a reason for hiding this comment

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

Space between { and | detected.

end

handle_asynchronously :start_reminder, run_at: proc { |i| i.when_to_run_start_reminder }
handle_asynchronously :reminder, run_at: proc { |i| i.when_to_run_reminder }
Copy link

Choose a reason for hiding this comment

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

Space between { and | detected.


handle_asynchronously :start_reminder, run_at: proc { |i| i.when_to_run_start_reminder }
handle_asynchronously :reminder, run_at: proc { |i| i.when_to_run_reminder }
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing.

}.to change(Delayed::Job.count).by(1)
end
end
end
Copy link

Choose a reason for hiding this comment

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

Trailing whitespace detected.

end

it 'enqueues remainder email in delayed job queue' do
expect {
Copy link

Choose a reason for hiding this comment

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

Avoid using {...} for multi-line blocks.

end

it 'enqueues remainder job in delayed job queue' do
expect {
Copy link

Choose a reason for hiding this comment

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

Avoid using {...} for multi-line blocks.


it 'enqueues remainder email in delayed job queue' do
expect {
delay.reminder()
Copy link

Choose a reason for hiding this comment

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

Do not use parentheses for method calls with no arguments.


it 'enqueues remainder job in delayed job queue' do
expect {
delay.start_reminder()
Copy link

Choose a reason for hiding this comment

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

Do not use parentheses for method calls with no arguments.

@codeclimate
Copy link

codeclimate bot commented Mar 22, 2021

Code Climate has analyzed commit 0eb1e78 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 37.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 29.1% (-21.7% change).

View more on Code Climate.

class DueDateTest < ActiveSupport::TestCase
describe "due_date_reminder_functions" do
before(:all) do
@due_at = DateTime.current - 4.days
Copy link

Choose a reason for hiding this comment

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

Prefer Date or Time over DateTime.

class DueDateTest < ActiveSupport::TestCase
describe "due_date_reminder_functions" do
before(:all) do
@due_at = Date.today - 4.days
Copy link

Choose a reason for hiding this comment

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

Do not use Date.today without zone. Use Time.zone.today instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants