Skip to content
This repository was archived by the owner on Jul 24, 2020. It is now read-only.

Improve Emails (used to be 317 checkin emails) #519

Merged
merged 27 commits into from
Jul 3, 2014

Conversation

quinnzhang
Copy link

Per issue 317, when a user returns a piece of overdue equipment, UserMailer sends an email to the user with the name of the equipment, its notes, and the late fee. AdminMailer sends a very similar email to the admin with the same information.

@orenyk orenyk added this to the 3.2.0 milestone May 23, 2014
@orenyk orenyk self-assigned this May 23, 2014
@@ -18,6 +18,10 @@
float:right;
}

#model_name {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to belong here; I think this was a fix to a small bug with cart positioning. I think I'll take this change out of this pull request, make an issue for the cart positioning, and add a new branch with this change for the that issue. @caseywatts does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find an element with this ID anywhere in the cart sidebar. I'm going to simply remove this code for now since it is both unrelated to this issue and seems to do nothing; we can always revert / add this later if necessary.


desc "Send email to admin on overdue reservations checked in with fine"
task :send_overdue_checked_in_fine_admin => :environment do
overdue_checked_in = Reservation.where("checked_out IS NOT NULL and checked_in IS NOT NULL and due_date < checked_in")
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this send an e-mail for every past overdue reservation (e.g. every old reservation that was checked in overdue) whenever the rake tasks are run? We need this to be sent only once... not sure how to test this.

@orenyk
Copy link
Contributor

orenyk commented May 25, 2014

Ok, I just tried testing this in my local development environment and it seems to be broken. It's definitely registering false positives, in that it's sending e-mails for reservations that were returned on time, but more critically, the e-mails (particularly the admin ones) are not nearly detailed enough. We should be providing the entire timeline of the reservation, from creation, start date, check-out, due date, and check-in, along with the actual user who made the reservation, and maybe the checkout persons who checked the equipment in and out. I'm bumping this pull request to the next milestone, here's a to-do list before release:

  • Write tests for sending e-mails only for past overdue reservations, as well as only sending them once per reservation
  • Fix the false positives in the rake task
  • Implement some way to ensure that we're not sending multiple e-mails per reservation
  • Make both admin and user e-mails more informative

Let's make sure that this feature is both well thought out and well tested!

@orenyk orenyk modified the milestones: 3.3.0, 3.2.0 May 25, 2014
@orenyk orenyk removed their assignment May 25, 2014
@squidgetx squidgetx self-assigned this Jun 12, 2014
@squidgetx
Copy link
Contributor

Emails sent for 'Checking in overdue items' that should only be sent once per day:

I think if we check that the check-in date was yesterday (and that the item was overdue), the email should only send once per reservation (assuming that the rake task is only run once per day, which is what it's currently set at in config/schedule.rb)

We should also combine the new email into one email and put it as one of the customizable emails in appconfig

Actually, we should just combine the user mailer and the admin mailer into one mailer... I don't see why we need two classes to send emails. Can't we just have one class that has different methods for mailing to different users..? Anyway, that's gonna be a bit too much trouble to sort out just now, but what we can do is at least just cc the admin in the user mail instead of having 2 email templates

Merge conflicts suck, since this is a pretty old issue I'm going to merge dev into it just to keep it up to date moving forward

Also, it seems that we have two different issues relating to emails.. (#541)..

@squidgetx squidgetx mentioned this pull request Jun 13, 2014
2 tasks
@squidgetx squidgetx removed their assignment Jun 17, 2014
@squidgetx
Copy link
Contributor

Merged #541
Tasks

Currently, when a piece of equipment is checked in or out an email is sent to the Bass Media delegate account that includes the patron's name, the equipment checked out, the due date, and any Reservation Notes that were submitted. In addition to this, it would speed Equipment Specialist workflow immensely if the name of the circ desk worker that checked the equipment out and in were included. Currently doing this requires the Eq Spec to sign into reservations to locate the specific reservation. We've had numerous student workers request this feature (#449)

  • Overdue checkin email
  • Make sure rake task is running correctly
  • Write mailer tests

Edit:
Wait, why are we sending these messages from the mailman rake task instead of from the app controller??

@squidgetx
Copy link
Contributor

Emails that are currently disabled/commented out

  • Checkin/checkout receipts
  • Reservation confirmations

What do we want to do with these? The commented out lines are for buttons that (I presume) the staff would be able to push to send an email confirmation for reservation/checkin/checkout to the user.

@squidgetx squidgetx changed the title 317 overdue checkin emails Improve Emails (used to be 317 checkin emails) Jun 19, 2014
@squidgetx squidgetx mentioned this pull request Jun 27, 2014
@squidgetx
Copy link
Contributor

This is ready to go~

"@equipment_list@\n\n"\
"If the equipment is returned after 4 pm on @return_date@ you will be charged a late fee or replacement fee. Repeated late returns will result in the privilege to make further reservations for the rest of the term to be revoked.\n\n"\
"If you fail to return your equipment on time the curse of @department_name@ will be placed upon you and your kin for 7 generations. Also, you will have to pay a late fee of @late_fee@ per day. If you have lost the item you may have to pay a replacement fee and/or sacrifice your first born child.\n"\
Copy link
Contributor

Choose a reason for hiding this comment

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

please change this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aww. you know this is just the default we ship to new clients and that the admin can write their own template, right?

Anyway, I changed it.

@orenyk
Copy link
Contributor

orenyk commented Jul 3, 2014

Looks good, nice work!

orenyk added a commit that referenced this pull request Jul 3, 2014
Improve Emails (used to be 317 checkin emails)
@orenyk orenyk merged commit 375ba77 into development Jul 3, 2014
@squidgetx squidgetx deleted the 317_overdue_checkin_emails branch July 3, 2014 16:10
@orenyk orenyk mentioned this pull request Jul 8, 2014
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants