Permalink
Browse files

Correct error causing misleading validation errors to fire on event edit

* Suggestions were displayed as one to many stored as one to one
* As result form was firing input required validation errors on invisible fields
* Since the forms were invisible this was a nonsensical error to user
* Refactor Suggestion into two models Primary and Secondary Suggestion
* Add one to many relationship between Primary and Secondary Suggestions
* Migrate data from existing data model into new data model
* Update specs to handle/test new architecture
* Clean up forms use to submit edit and create event data
* App no longer uses forked version of cocoon gem
* Namespaced all javascript in the event create/edit path
* https://trello.com/c/i9LzoLr5
  • Loading branch information...
1 parent a7dad71 commit 3d89e6393e3e959a2ec14a7fec982c7ac66aba59 @brittballard brittballard committed Jan 30, 2013
Showing with 694 additions and 274 deletions.
  1. +1 −0 .gitignore
  2. +1 −6 Gemfile
  3. +2 −8 Gemfile.lock
  4. +15 −14 app/assets/javascripts/jquery.app.js
  5. +14 −6 app/assets/javascripts/votes.coffee
  6. +2 −3 app/controllers/votes_controller.rb
  7. +5 −3 app/decorators/event_decorator.rb
  8. +2 −2 app/mailers/user_mailer.rb
  9. +21 −7 app/models/event.rb
  10. +23 −0 app/models/primary_suggestion.rb
  11. +22 −0 app/models/secondary_suggestion.rb
  12. +0 −18 app/models/suggestion.rb
  13. +6 −1 app/models/user.rb
  14. +10 −12 app/models/vote.rb
  15. +13 −0 app/views/events/_checkbox.html.erb
  16. +0 −15 app/views/events/_existing_suggestion_fields.html.erb
  17. +11 −9 app/views/events/_form.html.erb
  18. +59 −42 app/views/events/_grid.html.erb
  19. +11 −4 app/views/events/_primary_suggestion_fields.html.erb
  20. +9 −5 app/views/events/_secondary_suggestion_fields.html.erb
  21. +12 −16 app/views/events/_vote.html.erb
  22. +3 −0 app/views/events/_vote_count.html.erb
  23. +22 −0 app/views/events/_vote_form.html.erb
  24. +5 −1 app/views/user_mailer/_list_votes.html.erb
  25. +1 −1 app/views/user_mailer/vote_confirmation.text.erb
  26. +9 −0 db/migrate/20130130221107_create_secondary_suggestions.rb
  27. +9 −0 db/migrate/20130130221120_create_primary_suggestions.rb
  28. +6 −0 db/migrate/20130130221716_add_suggestion_to_votes.rb
  29. +8 −0 db/migrate/20130131222604_remove_non_null_from_suggestion_id_on_vote_table.rb
  30. +5 −0 db/migrate/20130201003044_add_event_id_to_votes.rb
  31. +6 −0 db/migrate/20130201032306_add_suggestion_id_to_primary_and_secondary_suggestion.rb
  32. +96 −0 db/migrate/20130201033116_migrate_data_into_new_two_table_architecture.rb
  33. +8 −0 db/migrate/20130205014728_add_indexs_to_primary_and_secondary_suggestions.rb
  34. +11 −0 db/migrate/20130205230425_add_non_null_to_vote_suggestion_fields.rb
  35. +35 −10 db/schema.rb
  36. +2 −2 spec/acceptance/step_definitions/event_creation_steps.rb
  37. +1 −1 spec/acceptance/step_definitions/form_steps.rb
  38. +18 −3 spec/acceptance/step_definitions/voting_steps.rb
  39. +35 −4 spec/acceptance/user_can_vote_on_a_suggestion.feature
  40. +30 −7 spec/decorators/event_decorator_spec.rb
  41. +12 −6 spec/factories.rb
  42. +2 −1 spec/javascripts/events_spec.js
  43. +4 −3 spec/javascripts/votes_spec.js
  44. +0 −1 spec/mailers/user_mailer_spec.rb
  45. +16 −8 spec/models/event_spec.rb
  46. +41 −0 spec/models/primary_suggestion_spec.rb
  47. +49 −0 spec/models/secondary_suggestion_spec.rb
  48. +0 −31 spec/models/suggestion_spec.rb
  49. +13 −10 spec/models/vote_spec.rb
  50. +7 −13 spec/support/voting.rb
  51. +1 −1 vendor/assets/javascripts/vendor.js
View
@@ -28,3 +28,4 @@ bin/
coverage
.tddium*
vendor/cache
+.foreman
View
@@ -26,12 +26,7 @@ gem 'tddium'
gem 'thin'
gem 'yam', '0.0.5'
gem 'zclip-rails'
-
-# We currently use this forked repo as it allows you to nest links to add
-# associations. We can move back to the original gem when the gem fixes a bug
-# where nesting causes infinite looping and the gem correctly adds nested
-# dynamically added objects to the params.
-gem 'cocoon', git: 'git://github.com/jsteiner/cocoon.git', branch: 'scheddo'
+gem 'cocoon'
# Gems used only for assets, not required in production environments by default.
group :assets do
View
@@ -1,10 +1,3 @@
-GIT
- remote: git://github.com/jsteiner/cocoon.git
- revision: ab64ff6e465793343744f20a2a36295f80ad63de
- branch: scheddo
- specs:
- cocoon (1.0.20)
-
GEM
remote: https://rubygems.org/
specs:
@@ -66,6 +59,7 @@ GEM
childprocess (0.3.6)
ffi (~> 1.0, >= 1.0.6)
cocaine (0.4.2)
+ cocoon (1.1.2)
coderay (1.0.8)
coffee-rails (3.2.2)
coffee-script (>= 2.2.0)
@@ -320,7 +314,7 @@ DEPENDENCIES
bourne
bundler (>= 1.2.0.pre)
capybara-webkit (~> 0.12.0)
- cocoon!
+ cocoon
coffee-rails (~> 3.2)
database_cleaner
delayed_job_active_record
@@ -1,13 +1,14 @@
- var setNextDatePicker = function () {
- nextDateField = $(this)
- .parent()
- .parent()
- .parent()
- .next()
- .find('input[data-role="primary-suggestion"]');
- nextDate = $(this).datepicker('getDate');
- nextDate.setDate(nextDate.getDate()+1);
- $(nextDateField).datepicker('option', 'defaultDate', nextDate);
+Namespaced.declare('Scheddo');
+
+Scheddo.setNextDatePicker = function() {
+ nextDateField = $(this).parent().
+ parent().
+ parent().
+ next().
+ find('input[data-role="primary-suggestion"]');
+ nextDate = $(this).datepicker('getDate');
+ nextDate.setDate(nextDate.getDate()+1);
+ $(nextDateField).datepicker('option', 'defaultDate', nextDate);
}
$(document).ready(function() {
@@ -29,7 +30,7 @@ $(document).ready(function() {
prevText: '',
dateFormat: 'D, M dd yy',
constrainInput: false,
- onSelect: setNextDatePicker
+ onSelect: Scheddo.setNextDatePicker
});
};
@@ -83,7 +84,7 @@ $(document).ready(function() {
var forms = $('form[id*=new_event], form[id*="edit_event"]');
forms.find('div.nested-fields input').removeAttr('maxlength');
- forms.bind('insertion-callback', function(){
+ forms.bind('cocoon:after-insert', function(){
datepicker();
addRemovalAnimation();
showRemoveIcons();
@@ -103,14 +104,14 @@ $(document).ready(function() {
bind_to_new_time_fields();
bind_to_changed_primary_fields();
- $('#new_event ol').bind('insertion-callback', function() {
+ $('#new_event ol').bind('cocoon:after-insert', function() {
set_default_date();
bind_to_new_time_fields();
bind_to_changed_primary_fields();
});
function bind_to_new_time_fields() {
- $('.nested-fields.primary').bind('insertion-callback', function() {
+ $('.nested-fields.primary').bind('cocoon:after-insert', function() {
var primary_suggestions = $(this).find('[data-role="primary-suggestion"]');
var primary_suggestion_value = primary_suggestions.first().val();
primary_suggestions.last().val(primary_suggestion_value);
@@ -1,19 +1,27 @@
+Namespaced.declare('Scheddo')
+
$('.votable').delegate '.vote[data-role=create]', 'ajax:success', (e, data, textStatus, jqXHR) ->
- voteCallback.call(this, data)
+ Scheddo.voteCallback.call(this, data)
+
$('.votable').delegate '.vote[data-role=delete]', 'ajax:success', (e, data, textStatus, jqXHR) ->
- unvoteCallback.call(this, data)
+ Scheddo.unvoteCallback.call(this, data)
+
$('.votable').delegate '.vote', 'ajax:error', (e, data, textStatus, jqXHR) ->
- voteErrorCallback.call(this, data)
+ Scheddo.voteErrorCallback.call(this, data)
+
$('.votable').delegate '.vote', 'ajax:beforeSend', (e, data, textStatus, jqXHR) ->
if $('body').hasClass('loading')
return false
$('body').addClass('loading')
+$('.votable').on 'click', (e, data, status, jqXhr) ->
+
-window.voteCallback = (data) ->
+Scheddo.voteCallback = (data) ->
$('body').removeClass('loading')
$(this).attr('data-role','delete')
$(this).attr('action',"/votes/#{data.vote.id}")
+ $(this).find('#vote_id').val(data.vote.id)
$(this).append($('<input name=_method type=hidden value=delete />'))
if($(this).data('hover'))
$(this).one 'mouseout', ->
@@ -24,7 +32,7 @@ window.voteCallback = (data) ->
$(".vote-count[data-id=#{id}]").text(parseInt($(".vote-count[data-id=#{id}]").text()) + 1 )
-window.unvoteCallback = (data) ->
+Scheddo.unvoteCallback = (data) ->
$('body').removeClass('loading')
$(this).attr('data-role','create')
$(this).attr('action','/votes')
@@ -37,7 +45,7 @@ window.unvoteCallback = (data) ->
id = $(this).parent().data('id')
$(".vote-count[data-id=#{id}]").text(parseInt($(".vote-count[data-id=#{id}]").text()) - 1 )
-window.voteErrorCallback = (data) ->
+Scheddo.voteErrorCallback = (data) ->
$('body').removeClass('loading')
$(".flash").append($("<div id=flash-error>#{data.statusText}</div>"))
@@ -17,12 +17,11 @@ def create
end
def destroy
- suggestion = Suggestion.find(params[:vote][:suggestion_id])
- vote = current_user.vote_for_suggestion(suggestion)
+ vote = current_user.votes.find(params[:vote][:id])
vote.destroy
respond_to do |format|
- format.html { redirect_to suggestion.event }
+ format.html { redirect_to vote.event }
format.json { render json: { status: :ok } }
end
end
@@ -2,8 +2,10 @@ class EventDecorator < Draper::Base
decorates :event
def build_suggestions
- suggestions[0] ||= Suggestion.new
- suggestions[1] ||= Suggestion.new
+ primary_suggestions[0] ||= PrimarySuggestion.new
+ primary_suggestions[1] ||= PrimarySuggestion.new
+ primary_suggestions[0].secondary_suggestions[0] ||= SecondarySuggestion.new
+ primary_suggestions[1].secondary_suggestions[0] ||= SecondarySuggestion.new
end
def first_invitee_for_invitation
@@ -43,7 +45,7 @@ def user_voted?(user)
end
def user_votes(user)
- user.votes.joins(:suggestion).where(suggestions: { event_id: self } )
+ user.votes.where(event_id: self)
end
private
@@ -41,7 +41,7 @@ def reminder(invitation, sender)
def vote_confirmation(vote)
@user = vote.voter
- @event = EventDecorator.new(vote.suggestion.event)
+ @event = EventDecorator.new(vote.event)
mail(
to: @user.email,
@@ -51,7 +51,7 @@ def vote_confirmation(vote)
def vote_notification(vote)
@voter = vote.voter
- @event = EventDecorator.new(vote.suggestion.event)
+ @event = EventDecorator.new(vote.event)
mail(
to: @event.owner.email,
View
@@ -1,19 +1,20 @@
class Event < ActiveRecord::Base
NAME_MAX_LENGTH = 70
- attr_accessible :name, :suggestion, :suggestions_attributes, :uuid
+ attr_accessible :name, :primary_suggestions_attributes, :uuid
belongs_to :owner, foreign_key: 'user_id', class_name: 'User'
- has_many :suggestions
- has_many :votes, through: :suggestions
+ has_many :primary_suggestions
+ has_many :votes
has_many :invitations
has_many :users, through: :invitations, source: :invitee, source_type: 'User'
has_many :guests, through: :invitations, source: :invitee, source_type: 'Guest'
has_many :groups, through: :invitations, source: :invitee, source_type: 'Group'
- accepts_nested_attributes_for :suggestions, reject_if: :all_blank,
+ accepts_nested_attributes_for :primary_suggestions,
+ reject_if: :reject_primary_suggestion?,
allow_destroy: true
before_validation :generate_uuid, on: :create
@@ -39,6 +40,10 @@ def invitees
(users + guests).sort { |a, b| b.created_at <=> a.created_at }
end
+ def suggestions
+ primary_suggestions
+ end
+
def to_param
uuid
end
@@ -50,12 +55,15 @@ def generate_uuid
end
def set_first_suggestion
- suggestions[0] ||= Suggestion.new
+ primary_suggestions[0] ||= PrimarySuggestion.new
end
def add_errors_if_no_suggestions
if lacks_suggestions?
- errors.add(:suggestions, 'An event must have at least one suggestion')
+ errors.add(
+ :primary_suggestions,
+ 'An event must have at least one suggestion'
+ )
end
end
@@ -64,7 +72,7 @@ def lacks_suggestions?
end
def remaining_suggestions
- suggestions.reject do |s|
+ primary_suggestions.reject do |s|
s.marked_for_destruction?
end
end
@@ -81,4 +89,10 @@ def enqueue_event_created_jobs
def invitations_without(user)
invitations.reject { |i| i.invitee == user }
end
+
+ def reject_primary_suggestion?(attributes)
+ attributes['id'].nil? &&
+ attributes['description'].empty? &&
+ attributes['secondary_suggestions_attributes']['0']['description'].empty?
+ end
end
@@ -0,0 +1,23 @@
+# Generally used to represent a date
+
+class PrimarySuggestion < ActiveRecord::Base
+ attr_accessible :description, :secondary_suggestions_attributes
+
+ validates :description, presence: { message: 'This field is required' }
+
+ belongs_to :event
+ has_many :secondary_suggestions
+ has_many :votes, as: :suggestion, dependent: :destroy
+
+ accepts_nested_attributes_for :secondary_suggestions,
+ reject_if: :all_blank,
+ allow_destroy: true
+
+ def vote_count
+ votes.count
+ end
+
+ def full_description
+ description
+ end
+end
@@ -0,0 +1,22 @@
+# Generally used to represent a time
+
+class SecondarySuggestion < ActiveRecord::Base
+ attr_accessible :description
+
+ validates :description, presence: { message: 'This field is required' }
+
+ belongs_to :primary_suggestion
+ has_many :votes, as: :suggestion, dependent: :destroy
+
+ def vote_count
+ votes.count
+ end
+
+ def event
+ primary_suggestion.event
+ end
+
+ def full_description
+ "#{primary_suggestion.description}, #{description}"
+ end
+end
View
@@ -1,18 +0,0 @@
-class Suggestion < ActiveRecord::Base
- validates :primary, presence: { message: 'This field is required' }
-
- attr_accessible :primary, :secondary, :event
-
- belongs_to :event
- has_many :votes, dependent: :destroy
-
- def vote_count
- persisted_votes.size
- end
-
- private
-
- def persisted_votes
- votes.select(&:persisted?)
- end
-end
View
@@ -61,7 +61,12 @@ def to_s
end
def vote_for_suggestion(suggestion)
- votes.find_by_suggestion_id(suggestion.id)
+ votes.
+ where(
+ suggestion_id: suggestion.id,
+ suggestion_type: suggestion.class.name
+ ).
+ first
end
def voted_for_suggestion?(suggestion)
Oops, something went wrong.

0 comments on commit 3d89e63

Please sign in to comment.