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 the email to event feature #54

Merged
merged 5 commits into from Jun 19, 2017

Conversation

Projects
None yet
3 participants
@nikhilgupta1211
Contributor

nikhilgupta1211 commented Jun 15, 2017

Created an EventMailer, defined an action email_event in the events_controller and created views for event_info method in the Event mailer.

Removed the email action in the events_controller as it was not necessary

@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.1%) to 92.597% when pulling 8f26f14 on nikhilgupta1211:emailevent into 36bf684 on openSUSE:master.

@openSUSE openSUSE deleted a comment from coveralls Jun 16, 2017

@openSUSE openSUSE deleted a comment from coveralls Jun 16, 2017

@openSUSE openSUSE deleted a comment from coveralls Jun 16, 2017

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 16, 2017

The width of the input field is currently 206px. It should be wider. Maybe 600 or even more.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 16, 2017

Also the submit and recepients select buttons are far on right. The should be close to the form.

@nikhilgupta1211

This comment has been minimized.

Contributor

nikhilgupta1211 commented Jun 16, 2017

The email view is based on Bootstrap 3 so as soon as we merge #53 these issues will be fixed automatically :)

@@ -141,7 +141,7 @@ def initialize(user)
can :manage, Budget
# Events
can [:update, :validate, :email], Event
can [:update, :validate, :email, :email_event], Event

This comment has been minimized.

@bgeuken

bgeuken Jun 16, 2017

Member

Please add a test for this, like in the other PR.

This comment has been minimized.

@nikhilgupta1211
@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage decreased (-0.4%) to 92.075% when pulling 5ccff2e on nikhilgupta1211:emailevent into 3333581 on openSUSE:master.

@openSUSE openSUSE deleted a comment from coveralls Jun 18, 2017

click_button("Select recipients")
find('a', :text => "Incomplete").click
page.should have_field('To', with: 'gial.ackbar@rebel-alliance.org,luke.skywalker@rebel-alliance.org,evram.lajaie@rebel-alliance.org,c3po@droids.com')

This comment has been minimized.

@bgeuken

bgeuken Jun 18, 2017

Member

Let's shorten this test by deirectly going to the email view of that event. And we can also remove the selection of recipients, since we just want to test canceling the mail here.

@@ -73,12 +73,10 @@
click_link "Email"
page.should have_content "Email the participants of Death Star's destruction celebration"

This comment has been minimized.

@bgeuken

bgeuken Jun 18, 2017

Member

Let's simplifv this test by splitting it up.

Have one test that ensures events have a email link when the user has the tsp role. And that clicking the link directs the user to the email view.
That test will complement testing access to the page ("User logged in is not a tsp member" and "When a user is not logged in").

That way we can focus in this test on testing that a user can send emails. So here we can directly go to the email view and do the stuff that happens in line 77 and following.

I would recommend doing this via git rebase -i 704e604719d61c11f0f1174c4a5126aae76ba286^. Then change 'pick 704e604' to 'edit 704e604', do your changes, do a git add, and git rebase --continue.

@bgeuken

This comment has been minimized.

Member

bgeuken commented Jun 18, 2017

The 'To' field is IMO to big. It should have the same height like the Subject field. If there are more email addresses in the field, a user could still increase the field size.

nikhilgupta1211 added some commits Jun 14, 2017

Added the email to event feature
Created an EventMailer, defined an action email_event in the events_controller and created views for event_info method in the Event mailer
Removed protected from the email action
Created specs for EventMailer
Added the mailer specs directory and created event_spec.rb
@coveralls

This comment has been minimized.

coveralls commented Jun 18, 2017

Coverage Status

Coverage increased (+0.1%) to 92.611% when pulling 0410358 on nikhilgupta1211:emailevent into 3333581 on openSUSE:master.

@openSUSE openSUSE deleted a comment from coveralls Jun 19, 2017

@bgeuken bgeuken merged commit 0e14e82 into openSUSE:master Jun 19, 2017

2 checks passed

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