Skip to content

Commit

Permalink
Convert circular relationship into a has_one
Browse files Browse the repository at this point in the history
Cycles in object graphs are best avoided so convert the creator
signature association to a has_one association using a boolean field
to indicate that it's a creator.

To prevent duplicate creator signatures we add a partial unique index
on the signature petition_id column with a `creator = 't'` condition.
  • Loading branch information
pixeltrix committed Sep 11, 2017
1 parent bbff6aa commit 43f7ef2
Show file tree
Hide file tree
Showing 72 changed files with 247 additions and 227 deletions.
2 changes: 1 addition & 1 deletion app/controllers/admin/petition_details_controller.rb
Expand Up @@ -21,7 +21,7 @@ def fetch_petition
def petition_params
params.require(:petition).permit(
:action, :background, :additional_details,
:special_consideration, :creator_signature_attributes => [:name]
:special_consideration, :creator_attributes => [:name]
)
end
end
8 changes: 4 additions & 4 deletions app/controllers/petitions_controller.rb
Expand Up @@ -56,7 +56,7 @@ def create
assign_stage
@stage_manager = Staged::PetitionCreator.manager(petition_params_for_create, request, params[:stage], params[:move])
if @stage_manager.create_petition
@stage_manager.petition.creator_signature.store_constituency_id
@stage_manager.petition.creator.store_constituency_id
send_email_to_gather_sponsors(@stage_manager.petition)
redirect_to thank_you_petition_url(@stage_manager.petition)
else
Expand Down Expand Up @@ -167,12 +167,12 @@ def petition_params_for_create
params.
require(:petition).
permit(:action, :background, :additional_details, :duration,
creator_signature: [
creator: [
:name, :email, :email_confirmation,
:postcode, :location_code, :uk_citizenship
]).tap do |sanitized|
if sanitized['creator_signature'].present?
sanitized['creator_signature_attributes'] = sanitized.delete('creator_signature')
if sanitized['creator'].present?
sanitized['creator_attributes'] = sanitized.delete('creator')
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/sponsors_controller.rb
Expand Up @@ -5,7 +5,7 @@ class SponsorsController < SignaturesController

before_action :redirect_to_petition_page_if_moderated, except: [:thank_you, :signed]
before_action :redirect_to_moderation_info_page_if_sponsored, except: [:thank_you, :signed]
before_action :validate_creator_signature, only: [:new]
before_action :validate_creator, only: [:new]

def verify
if @signature.validated?
Expand Down Expand Up @@ -88,7 +88,7 @@ def redirect_to_moderation_info_page_if_sponsored
end
end

def validate_creator_signature
@petition.validate_creator_signature!
def validate_creator
@petition.validate_creator!
end
end
2 changes: 1 addition & 1 deletion app/helpers/page_title_helper.rb
Expand Up @@ -52,7 +52,7 @@ def options
opts[:petition] = petition.action

unless petition.is_a?(Archived::Petition)
opts[:creator] = petition.creator_signature.name
opts[:creator] = petition.creator.name
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/helpers/petition_helper.rb
Expand Up @@ -48,7 +48,7 @@ def render_petition_hidden_details(stage_manager, form)
concat hidden_field_tag(:stage, stage_manager.stage)
concat hidden_field_tag(:move, 'next')
concat render('/petitions/create/petition_details_hidden', petition: stage_manager.stage_object, f: form) unless stage_manager.stage == 'petition'
if stage_manager.stage_object.creator_signature.present?
if stage_manager.stage_object.creator.present?
concat render('/petitions/create/your_details_hidden', petition: stage_manager.stage_object, f: form) unless stage_manager.stage == 'creator'
concat render('/petitions/create/email_hidden', petition: stage_manager.stage_object, f: form) unless ['creator', 'replay-email'].include? stage_manager.stage
end
Expand Down
2 changes: 1 addition & 1 deletion app/jobs/notify_creators_that_moderation_is_delayed_job.rb
Expand Up @@ -3,7 +3,7 @@ class NotifyCreatorsThatModerationIsDelayedJob < ApplicationJob

def perform(subject, body)
petitions.find_each do |petition|
NotifyCreatorThatModerationIsDelayedJob.perform_later(petition.creator_signature, subject, body)
NotifyCreatorThatModerationIsDelayedJob.perform_later(petition.creator, subject, body)
end
end

Expand Down
Expand Up @@ -3,7 +3,7 @@ class NotifyCreatorsThatParliamentIsDissolvingJob < ApplicationJob

def perform
petitions.find_each do |petition|
NotifyCreatorThatParliamentIsDissolvingJob.perform_later(petition.creator_signature)
NotifyCreatorThatParliamentIsDissolvingJob.perform_later(petition.creator)
end
end

Expand Down
2 changes: 1 addition & 1 deletion app/jobs/notify_everyone_of_moderation_decision_job.rb
Expand Up @@ -4,7 +4,7 @@ class NotifyEveryoneOfModerationDecisionJob < ApplicationJob
end

def perform(petition)
creator = petition.creator_signature
creator = petition.creator
sponsors = petition.sponsors.validated

if petition.published?
Expand Down
4 changes: 2 additions & 2 deletions app/jobs/stop_petitions_early_job.rb
Expand Up @@ -26,9 +26,9 @@ def send_notification(petition)
unless petition.special_consideration?
case petition.state
when Petition::VALIDATED_STATE
NotifyCreatorOfValidatedPetitionBeingStoppedJob.perform_later(petition.creator_signature)
NotifyCreatorOfValidatedPetitionBeingStoppedJob.perform_later(petition.creator)
when Petition::SPONSORED_STATE
NotifyCreatorOfSponsoredPetitionBeingStoppedJob.perform_later(petition.creator_signature)
NotifyCreatorOfSponsoredPetitionBeingStoppedJob.perform_later(petition.creator)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/mailers/petition_mailer.rb
Expand Up @@ -91,7 +91,7 @@ def notify_creator_of_validated_petition_being_stopped(signature)
end

def gather_sponsors_for_petition(petition)
@petition, @creator = petition, petition.creator_signature
@petition, @creator = petition, petition.creator
mail to: @creator.email, subject: subject_for(:gather_sponsors_for_petition)
end

Expand Down
4 changes: 2 additions & 2 deletions app/mailers/sponsor_mailer.rb
Expand Up @@ -5,7 +5,7 @@ def sponsor_signed_email_below_threshold(petition, sponsor)

mail(
subject: "#{@sponsor.name} supported your petition",
to: @petition.creator_signature.email
to: @petition.creator.email
)
end

Expand All @@ -15,7 +15,7 @@ def sponsor_signed_email_on_threshold(petition, sponsor)

mail(
subject: "We’re checking your petition",
to: @petition.creator_signature.email
to: @petition.creator.email
)
end

Expand Down
24 changes: 9 additions & 15 deletions app/models/petition.rb
Expand Up @@ -33,7 +33,6 @@ class Petition < ActiveRecord::Base
has_perishable_token called: 'sponsor_token'

before_save :update_debate_state, if: :scheduled_debate_date_changed?
after_create :set_petition_on_creator_signature
after_create :update_last_petition_created_at

extend Searchable(:action, :background, :additional_details)
Expand Down Expand Up @@ -65,8 +64,8 @@ class Petition < ActiveRecord::Base
facet :overdue_in_moderation, -> { overdue_in_moderation.by_most_recent_moderation_threshold_reached }
facet :tagged_in_moderation, -> { tagged_in_moderation.by_most_recent_moderation_threshold_reached }

belongs_to :creator_signature, class_name: 'Signature'
accepts_nested_attributes_for :creator_signature, update_only: true
has_one :creator, -> { where(creator: true) }, class_name: 'Signature'
accepts_nested_attributes_for :creator, update_only: true

belongs_to :locked_by, class_name: 'AdminUser'

Expand All @@ -84,12 +83,12 @@ class Petition < ActiveRecord::Base
has_many :invalidations

include Staged::Validations::PetitionDetails
validates_presence_of :open_at, if: :open?
validates_presence_of :creator_signature, on: :create
validates_inclusion_of :state, in: STATES
validates :open_at, presence: true, if: :open?
validates :creator, presence: true
validates :state, inclusion: { in: STATES }

with_options allow_nil: true, prefix: true do
delegate :name, :email, to: :creator_signature, prefix: :creator
delegate :name, :email, to: :creator
delegate :code, :details, to: :rejection
delegate :summary, :details, :created_at, :updated_at, to: :government_response
delegate :date, :transcript_url, :video_url, :overview, to: :debate_outcome, prefix: :debate
Expand Down Expand Up @@ -309,7 +308,7 @@ def in_need_of_closing(time = Time.current)
end

def in_need_of_stopping(time = nil)
scope = preload(:creator_signature)
scope = preload(:creator)
time ? scope.stoppable.created_after(time) : scope.stoppable
end

Expand Down Expand Up @@ -589,9 +588,9 @@ def stop!(time = Time.current)
end
end

def validate_creator_signature!
def validate_creator!
if pending?
creator_signature && creator_signature.validate! && reload
creator && creator.validate! && reload
end
end

Expand Down Expand Up @@ -722,11 +721,6 @@ def closing_early_for_dissolution?(dissolution_at = Parliament.dissolution_at)
open_at && dissolution_at ? deadline > dissolution_at : false
end

# need this callback since the relationship is circular
def set_petition_on_creator_signature
creator_signature.update_attribute(:petition_id, id)
end

def cache_key(*timestamp_names)
case
when new_record?
Expand Down
6 changes: 1 addition & 5 deletions app/models/signature.rb
Expand Up @@ -234,10 +234,6 @@ def sanitized_name
name.to_s.parameterize
end

def creator?
petition.creator_signature == self
end

def pending?
state == PENDING_STATE
end
Expand Down Expand Up @@ -272,7 +268,7 @@ def validate!(now = Time.current)
retry_lock do
if pending?
update_signature_counts = true
petition.validate_creator_signature! unless creator?
petition.validate_creator! unless creator?

update_columns(
number: petition.signature_count + 1,
Expand Down
10 changes: 5 additions & 5 deletions app/models/staged/base/creator_signature.rb
Expand Up @@ -10,7 +10,7 @@ def initialize(petition)

delegate :id, :to_param, :model_name, :to_key, :name,
:email, :email?, :uk_citizenship, :postcode,
:location_code, :constituency, to: :creator_signature
:location_code, :constituency, to: :creator

def validation_context
:create
Expand All @@ -20,11 +20,11 @@ def validation_context

attr_reader :petition

def creator_signature
if petition.creator_signature.nil?
petition.build_creator_signature(location_code: 'GB')
def creator
if petition.creator.nil?
petition.build_creator(location_code: 'GB')
end
petition.creator_signature
petition.creator
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/staged/base/petition.rb
Expand Up @@ -11,7 +11,7 @@ def initialize(petition)

delegate :id, :to_param, :model_name, :to_key,
:action, :background, :additional_details,
:duration, :creator_signature, to: :petition
:duration, :creator, to: :petition
end
end
end
6 changes: 3 additions & 3 deletions app/models/staged/petition_creator.rb
Expand Up @@ -47,9 +47,9 @@ def stage_manager
end

def sanitize!
if petition.creator_signature
petition.creator_signature.email.strip! unless petition.creator_signature.email.blank?
petition.creator_signature.ip_address = @request.remote_ip
if petition.creator
petition.creator.email.strip! unless petition.creator.email.blank?
petition.creator.ip_address = @request.remote_ip
end
petition.action.strip! unless petition.action.blank?
end
Expand Down
14 changes: 7 additions & 7 deletions app/models/staged/petition_creator/has_creator_signature.rb
Expand Up @@ -4,20 +4,20 @@ module HasCreatorSignature
extend ActiveSupport::Concern

included do
validate :creator_signature_valid?
validate :creator_valid?

def creator_signature
@_creator_signature ||= self.class::CreatorSignature.new(petition)
def creator
@_creator ||= self.class::CreatorSignature.new(petition)
end

private

def creator_signature_valid?
if creator_signature.valid?
def creator_valid?
if creator.valid?
true
else
creator_signature.errors.each do |attribute, message|
attribute = "creator_signature.#{attribute}"
creator.errors.each do |attribute, message|
attribute = "creator.#{attribute}"
errors[attribute] << message
errors[attribute].uniq!
end
Expand Down
8 changes: 4 additions & 4 deletions app/views/admin/petition_details/show.html.erb
Expand Up @@ -31,10 +31,10 @@
<%= f.text_area :additional_details, tabindex: increment, rows: 7, class: 'form-control', disabled: @petition.editing_disabled? %>
<% end %>
<%= form_row for: [f.object, :creator_signature_name] do %>
<%= f.fields_for :creator_signature do |c| %>
<%= form_row for: [f.object, :creator_name] do %>
<%= f.fields_for :creator do |c| %>
<%= c.label :name, "Creator", class: 'form-label' %>
<%= error_messages_for_field @petition.creator_signature, :name %>
<%= error_messages_for_field @petition.creator, :name %>
<%= c.text_field :name, tabindex: increment, rows: 8, class: 'form-control', disabled: @petition.editing_disabled? %>
<% end %>
<% end %>
Expand All @@ -59,4 +59,4 @@

</div>

<%= render 'edit_lock' %>
<%= render 'edit_lock' %>
4 changes: 2 additions & 2 deletions app/views/admin/petitions/_petition_details.html.erb
Expand Up @@ -9,8 +9,8 @@

<dt>Creator</dt>
<dd>
<%= @petition.creator_signature.name %><br />
<%= auto_link(@petition.creator_signature.email) %>
<%= @petition.creator.name %><br />
<%= auto_link(@petition.creator.email) %>
</dd>

<% if @petition.in_todo_list? %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/petitions/index.html.erb
Expand Up @@ -55,7 +55,7 @@
<% @petitions.each do |petition| -%>
<tr class="petition-list-petition petition-list-petition-state-<%= petition.state.dasherize %>">
<td class="petition-list-petition-action"><%= link_to petition.action, admin_petition_path(petition) %></td>
<td class="petition-list-petition-creator"><%= mail_to petition.creator_signature.email, petition.creator_signature.name %></td>
<td class="petition-list-petition-creator"><%= mail_to petition.creator.email, petition.creator.name %></td>
<td class="petition-list-petition-id"><%= petition.id %></td>
<td><%= petition.state.humanize %></td>
<td class="date"><%= date_format(petition.deadline) %></td>
Expand Down
2 changes: 1 addition & 1 deletion app/views/petitions/_open_petition_show.html.erb
Expand Up @@ -35,7 +35,7 @@

<ul class="petition-meta">
<li class="meta-created-by">
<span class="label">Created by</span> <%= petition.creator_signature.name %>
<span class="label">Created by</span> <%= petition.creator.name %>
</li>
<li class="meta-deadline">
<% if petition.closing_early_for_dissolution? %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/petitions/create/_email_hidden.html.erb
@@ -1,3 +1,3 @@
<%= f.fields_for :creator_signature, petition.creator_signature do |csf| %>
<%= f.fields_for :creator, petition.creator do |csf| %>
<%= csf.hidden_field :email %>
<% end %>
2 changes: 1 addition & 1 deletion app/views/petitions/create/_replay_email_ui.html.erb
Expand Up @@ -2,7 +2,7 @@

<h1 class="page-title">Make sure this is right</h1>

<%= f.fields_for :creator_signature, petition.creator_signature do |csf| %>
<%= f.fields_for :creator, petition.creator do |csf| %>
<%= render 'petitions/create/signature_email', f: csf, signature: csf.object, hide_label: true %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/petitions/create/_your_details_hidden.html.erb
@@ -1,3 +1,3 @@
<%= f.fields_for :creator_signature, petition.creator_signature do |csf| %>
<%= f.fields_for :creator, petition.creator do |csf| %>
<%= render 'petitions/create/signature_hidden', f: csf, signature: csf.object %>
<% end %>
2 changes: 1 addition & 1 deletion app/views/petitions/create/_your_details_ui.html.erb
Expand Up @@ -5,7 +5,7 @@

<%= render 'petitions/create/error_summary', f: f %>
<%= f.fields_for :creator_signature, petition.creator_signature do |csf| %>
<%= f.fields_for :creator, petition.creator do |csf| %>
<%= render 'petitions/create/signature_form', :f => csf, :signature => csf.object, :context => 'create-petition' %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/petitions/gathering_support.html.erb
@@ -1,5 +1,5 @@
<h1>This petition is gathering support</h1>

<p><%= @petition.creator_signature.name %>’s petition needs <%= Site.minimum_number_of_sponsors %> supporters before we will check that it meets the <%= link_to "petition standards", help_path(anchor: 'standards') %> and publish it.</p>
<p><%= @petition.creator.name %>’s petition needs <%= Site.minimum_number_of_sponsors %> supporters before we will check that it meets the <%= link_to "petition standards", help_path(anchor: 'standards') %> and publish it.</p>

<p>Please try again in a few days.</p>
2 changes: 1 addition & 1 deletion app/views/petitions/moderation_info.html.erb
@@ -1,6 +1,6 @@
<h1>We’re checking this petition</h1>

<p><%= Site.minimum_number_of_sponsors %> people have already supported <%= @petition.creator_signature.name %>’s petition.</p>
<p><%= Site.minimum_number_of_sponsors %> people have already supported <%= @petition.creator.name %>’s petition.</p>

<p>We need to check it meets the <%= link_to "petition standards", help_path(anchor: 'standards') %> before we publish it.</p>

Expand Down

0 comments on commit 43f7ef2

Please sign in to comment.