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

Added Email index and show view #56

Merged
merged 4 commits into from Jun 23, 2017

Conversation

Projects
None yet
3 participants
@nikhilgupta1211
Contributor

nikhilgupta1211 commented Jun 21, 2017

No description provided.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 21, 2017

The breadcrumbs are missing;-)

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-1.0%) to 91.405% when pulling e192b35 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@@ -0,0 +1,7 @@
class EmailEvent < ActiveRecord::Base

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

Please rename it to EventEmail

@@ -0,0 +1,12 @@
class Createemailevents < ActiveRecord::Migration

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

Classes should be written in CamelCase

@email_event[:to].split(',').each do |e|
user = User.find_by(email: e)
if user.nil?
flash[:error] = 'Mail did not delivered to ' + e + ', because this email is not registered with TSP'

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

I would rephrase the first part to "Email was not delivered to". And maybe put the second part into a separate sentence, like "This email is not registered with TSP."

@email_event = EmailEvent.find(params[:id])
end
def event

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

What is this action used for?

@@ -4,22 +4,6 @@ class EventsController < InheritedResources::Base
skip_load_and_authorize_resource only: [:index, :show]

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

This commit and the previous one seem to belong together (Moving files and code). In such a case it makes sense to have the changes in one commit.
Please squash this commit into the previous one.

it { should be_able_to(:email, events(:yavin_hackaton)) }
it { should be_able_to(:email, events(:party)) }
it { should be_able_to(:email_event, events(:yavin_hackaton)) }
it { should be_able_to(:email_event, events(:party)) }

This comment has been minimized.

@bgeuken

bgeuken Jun 21, 2017

Member

I guess we should add tests for managing event mails instead

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.9%) to 91.473% when pulling 8752970 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

Created a model EventEmail
Generated a migration to create a table event_emails and defined a belongs_to association with Event and User
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.9%) to 91.473% when pulling 3f2d7ed on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

validates :to, presence: true
validates :subject, presence: true, length: { maximum: 150 }
validates :body, presence: true

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

We are using shoulda matchers which provides some nice methods for testing validations.
Please add them to the event mail tests.

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Nevermind. I just saw your other commit

word-wrap: break-word;
}
.labl {

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Please rename this one. If it's a label for some specific group of html elements call it *-label:-)

@@ -1,9 +1,11 @@
#
# CanCan permissions
#
# rubocop:disable ClassLength

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Since this is meant for the whole file, it's better to add this as an exception to the rubocp.yml file.

This comment has been minimized.

@nikhilgupta1211

nikhilgupta1211 Jun 22, 2017

Contributor

But if we add the file in rubocop.ym no other cops will also work on the file

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

You can exclude the cop for certain files with something like this:

MyCop:
  Exclude:                                                                                                                                                                
    - 'foo/bar/test.rb'
class Ability
include CanCan::Ability
# rubocop:disable MethodLength,AbcSize

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

You need to enable this cop again. Otherwise it stays disabled.

%h3 Subject:
.well.well-sm #{@event_email.subject}
%h3 Body:
.well #{@event_email.body}

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Please fix the indentation here.

it { should validate_presence_of :to }
it { should validate_presence_of :subject }
it { should validate_length_of(:subject).is_at_most(150) }
it { should validate_presence_of :body }

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Ah, you already did this 👍

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.9%) to 91.473% when pulling ab133a8 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

MethodLength:
Exclude:
- 'app/models/ability.rb'

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Tried it on my machine and it seems that only the initialize method offends that cop. Please just disable that cop.

AbcSize:
Exclude:
- 'app/models/ability.rb'

This comment has been minimized.

@bgeuken

bgeuken Jun 22, 2017

Member

Same here. Please just disable the cop for the single method, like you did before. Just re-enable the cop at the end of the method.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.803% when pulling 273a580 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 22, 2017

There is a bug in the breadcrumbs. When I create a email and then click on the event breadcrumb it directs me to a nonexistant page.
It looks like it uses the email id as event id.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 22, 2017

Another issue is that newlines I add to the email body get lost. So when I view a sent email it's a big blob of text, which makes it hard to read.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.745% when pulling 99d98a0 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.745% when pulling 99d98a0 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.793% when pulling 3f10d03 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.793% when pulling d6c09c1 on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.793% when pulling d8fb9eb on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

nikhilgupta1211 added some commits Jun 21, 2017

Created the controller event_emails to send mails for an event
Added the views for event_emails, defined a member route under events for event_emails and added permision for tsp member in ability.rb

Modified the events controller and views to implement the email event feature
Added feature specs for Event Email
Updated the event specs and added event_email_spec.rb
Added model tests for Event Email
Added a fixture event_emails.yml, created event_email_spec.rb and updated ability specs for Event Email
@coveralls

This comment has been minimized.

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.793% when pulling 998a1ac on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.6%) to 91.793% when pulling 998a1ac on nikhilgupta1211:emailindex into dde4cca on openSUSE:master.

@bgeuken bgeuken merged commit 4f8e8d7 into openSUSE:master Jun 23, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.6%) to 91.793%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment