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

Fix for contact-us when logged in - fixes #1394 #1399

Merged
merged 2 commits into from
Apr 20, 2018

Conversation

vyruss
Copy link
Contributor

@vyruss vyruss commented Apr 19, 2018

Fixes #1394 .

Changes proposed in this PR:

  • cater for logged in users

@vyruss vyruss self-assigned this Apr 19, 2018
@vyruss vyruss requested a review from briri April 19, 2018 19:16
Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

The changes should resolve the reported bug, but I suggest making some mods to give the user a better experience if an error is encountered.

redirect_to request.referrer
#render_new_page
unless user_signed_in?
unless verify_recaptcha(model: @contact) && @contact.save
Copy link
Contributor

Choose a reason for hiding this comment

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

We should technically do the @contact.save check regardless of whether or not the user is signed in. Would also be good to do a render instead of redirect on failure so that the user doesn't lose the text they've entered in the fields.
Maybe something like (pseudocode .... you might need to tweak):

@contact = ContactUs::Contact.new(params[:contact_us_contact])
flash[:alert] = nil

if !user_signed_in? && !verify_recaptcha(model: @contact)
  flash[:alert] = _('Captcha verification failed, please retry.')
end
if !flash[:alert].present? && @contact.save
  redirect_to(ContactUs.success_redirect || '/', :notice => _('Contact email was successfully sent.'))
else
  flash[:alert] = _('Unable to submit your request') unless flash[:alert].present?
  render 'new'
end

@vyruss
Copy link
Contributor Author

vyruss commented Apr 20, 2018

@briri done.

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

looks good. thanks for making those changes

@briri briri merged commit 1cbf169 into DMPRoadmap:master Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants