-
Notifications
You must be signed in to change notification settings - Fork 10
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
adds mailer and feedback form functionality #130
Conversation
if request.post? | ||
if validate | ||
RecordMailer.submit_feedback(params, request.remote_ip) | ||
flash[:success] = t('blacklight.feedback_form.success').html_safe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so html_safe
here does not appear to be helping with rendering tags correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove this since we're now calling #html_safe
in the overridden partial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Coverage decreased (-0.23%) when pulling 425a40be6f2fe257bb8027785670bf1e0feff53e on feedback into 9bd2efa on master. |
Coverage increased (+0.21%) when pulling ce4a2b299fe867098eca9cb4ae6fc24dae34bc46 on feedback into 094013e on master. |
else | ||
false | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be written a little more concisely as:
def show_feedback_form?
!controller.instance_of?(FeedbackFormsController)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a simple helper test may be in order (just in case this logic ever grows in complexity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
What's w/ the empty Under which condition do we get there (and should we try to prevent that)? |
@jkeck we don't anymore it seems... removing |
class RecordMailer < ActionMailer::Base | ||
|
||
def submit_feedback(params, ip) | ||
if !params[:name].blank? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think params[:name].present?
may read a little better. (as well as params[:to].present?
below)
That may just be my personal style, so I leave that 💯 % up to your discretion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
adds mailer and feedback form functionality
Fixes #37
Adds feedback form functionality to application with test suites. I am scoping out the quick book report button to another ticket as it may depend on #78
One issue is that the html rendering of the flash is not rendering html tags correctly. Any suggestions?
html_safe
does not seem to work.