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

Send notification email upon assignment/unassignment #10

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

eldriclim
Copy link
Owner

https://trello.com/c/SHmRJX2b

Why is this change necessary?

  • Deliverers are not aware of shift status at admin end

How does it address the issue?

  • Emails are sent to notify deliverers of assignment status

What side effects does this change have?

  • Undo assignment feature is added
  • Email traffic is simulated with 'letter-opener' gem

Deploy Notes

  • Migrations:
    • New column Email added to Deliverer
      • Emails of existing records are populated with "Phone@email.com"
  • Config:
    • Coveralls
      • Filters for app/mailers have been removed

@eldriclim eldriclim requested a review from leanhdaovn June 14, 2018 10:12
@eldriclim eldriclim self-assigned this Jun 14, 2018
@eldriclim eldriclim added enhancement New feature or request iteration-2 labels Jun 14, 2018
@coveralls
Copy link

coveralls commented Jun 14, 2018

Pull Request Test Coverage Report for Build 120

  • 59 of 63 (93.65%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 91.732%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/controllers/assignments_controller.rb 22 26 84.62%
Totals Coverage Status
Change from base Build 59: 0.1%
Covered Lines: 233
Relevant Lines: 254

💛 - Coveralls

db/schema.rb Outdated
@@ -25,10 +25,11 @@
create_table "deliverers", force: :cascade do |t|
t.string "name"
t.integer "vehicle"
t.integer "phone"
t.string "phone"
Copy link
Collaborator

Choose a reason for hiding this comment

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

which migration converts this column to string?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have changed it back to t.integer as migration for this change is suppose to be in PR #8 which is yet to be merged.

I'm unsure why the change is retained only for this branch though, could it be anomaly in the schema auto-generation?

@@ -8,6 +8,9 @@ class Deliverer < ApplicationRecord
validates :phone, presence: { message: 'Phone field is empty' },
numericality: { only_integer: true, message: 'Phone has to be an integer' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this validation still valid when the column type is changed to string?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am leaving it as it is for now since phone is now an integer

@@ -0,0 +1,58 @@
<h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is this view used?

Copy link
Owner Author

@eldriclim eldriclim Jun 25, 2018

Choose a reason for hiding this comment

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

Removed.
My bad, didn't remove when refractoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request iteration-2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants