ivaldi / brimir

Email helpdesk built using Ruby on Rails and Zurb Foundation
http://getbrimir.com
GNU Affero General Public License v3.0
1.38k stars 299 forks source link

Add validations when creating ticket from a web form #408

Closed mftaff closed 5 years ago

mftaff commented 6 years ago

~I added an attr_accessor to Ticket. When this attribute is true, it will skip the validations for content and subject.~

Hope this helps! I am happy to make infinite changes, so let me know if I missed anything/could do something better 😄

closes #386

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.003%) to 92.336% when pulling 13d9dbb3a467231754a5ab253dc632f5b8c8c0f9 on mftaff:ticket-validations into 04681f605f9c70c6863fed2c29fc0aa379075d0e on ivaldi:master.

frenkel commented 6 years ago

Looks great! I do think it's better to use "save contexts" for this instead of a virtual attribute. See the on argument in http://api.rubyonrails.org/classes/ActiveModel/Validations/ClassMethods.html#method-i-validate

mftaff commented 6 years ago

I see! That does seem like a cleaner solution.

Am I right in thinking that in order to implement this, we need to add a custom context (like :web-form ) whenever we are creating a ticket from a web_form? If so, I am having trouble finding where save/create is called for a web_form generated ticket? Is the method Ticket#save_with_label only called by the web_form? It seems not... Thanks for the help!

frenkel commented 6 years ago

Yes, a context like that would work (although - cannot be used in symbol names). If the save_with_label function is called from multiple places it would be best to add an argument to it that is filled by the controller (which knows whether it was a form submission).

mftaff commented 6 years ago

Awesome awesome. I think I found an even better solution by adding (!using_hook && !@ticket.valid?(:web_form)) to tickets_controller#can_create_a_ticket(using_hook):

def can_create_a_ticket(using_hook)
   if @ticket.nil? || !@ticket.valid? || (!using_hook && !@ticket.valid?(:web_form))
      flash.now[:alert] = I18n::translate(:form_validation_error)
      false
   ...

Make sense?

frenkel commented 6 years ago

Yes, that might work.

lostapathy commented 6 years ago

As a perhaps lighter-weight step ahead of this, would it make sense to add a validator that required at least one of [:subject, :content] be populated on create only?

It seems like we could do that without adding another context and without breaking any good behavior.

frenkel commented 6 years ago

Sure