From 1aefebdd0a56e0742baf86b150ac7a7442ff660d Mon Sep 17 00:00:00 2001 From: Kris Hill Date: Fri, 18 Sep 2026 09:23:19 -0700 Subject: [PATCH 1/5] [CMS-434] remove Forms files, never used in CMS --- app/assets/javascripts/cms/form_builder.js | 250 ------------------ app/assets/stylesheets/cms/default-forms.scss | 3 - .../cms/form_entries_controller.rb | 140 ---------- app/controllers/cms/form_fields_controller.rb | 74 ------ app/controllers/cms/forms_controller.rb | 35 --- app/models/cms/form.rb | 47 ---- app/models/cms/form_entry.rb | 75 ------ app/models/cms/form_field.rb | 78 ------ app/views/cms/form_entries/_buttons.html.erb | 2 - app/views/cms/form_entries/_form.html.erb | 7 - .../cms/form_entries/_internal_form.html.erb | 9 - app/views/cms/form_entries/edit.html.erb | 5 - app/views/cms/form_entries/error.html.erb | 3 - app/views/cms/form_entries/index.html.erb | 5 - app/views/cms/form_entries/new.html.erb | 5 - app/views/cms/form_entries/show.html.erb | 13 - app/views/cms/form_entries/submit.html.erb | 1 - app/views/cms/form_fields/_form.html.erb | 8 - app/views/cms/form_fields/_select.html.erb | 3 - app/views/cms/form_fields/_text_area.html.erb | 3 - .../cms/form_fields/_text_field.html.erb | 3 - app/views/cms/form_fields/edit.html.erb | 0 app/views/cms/form_fields/new.html.erb | 26 -- app/views/cms/form_fields/preview.html.erb | 16 -- app/views/cms/forms/_form.html.erb | 67 ----- app/views/cms/forms/render.html.erb | 15 -- app/views/cms/forms/show.html.erb | 6 - features/content_blocks/forms.feature | 28 -- spec/cms/form_entry_spec.rb | 153 ----------- spec/cms/form_fields_spec.rb | 143 ---------- spec/cms/form_spec.rb | 134 ---------- .../cms/form_entries_controller_test.rb | 176 ------------ .../cms/form_fields_controller_test.rb | 105 -------- test/functional/cms/forms_controller_test.rb | 120 --------- 34 files changed, 1758 deletions(-) delete mode 100644 app/assets/javascripts/cms/form_builder.js delete mode 100644 app/assets/stylesheets/cms/default-forms.scss delete mode 100644 app/controllers/cms/form_entries_controller.rb delete mode 100644 app/controllers/cms/form_fields_controller.rb delete mode 100644 app/controllers/cms/forms_controller.rb delete mode 100644 app/models/cms/form.rb delete mode 100644 app/models/cms/form_entry.rb delete mode 100644 app/models/cms/form_field.rb delete mode 100644 app/views/cms/form_entries/_buttons.html.erb delete mode 100644 app/views/cms/form_entries/_form.html.erb delete mode 100644 app/views/cms/form_entries/_internal_form.html.erb delete mode 100644 app/views/cms/form_entries/edit.html.erb delete mode 100644 app/views/cms/form_entries/error.html.erb delete mode 100644 app/views/cms/form_entries/index.html.erb delete mode 100644 app/views/cms/form_entries/new.html.erb delete mode 100644 app/views/cms/form_entries/show.html.erb delete mode 100644 app/views/cms/form_entries/submit.html.erb delete mode 100644 app/views/cms/form_fields/_form.html.erb delete mode 100644 app/views/cms/form_fields/_select.html.erb delete mode 100644 app/views/cms/form_fields/_text_area.html.erb delete mode 100644 app/views/cms/form_fields/_text_field.html.erb delete mode 100644 app/views/cms/form_fields/edit.html.erb delete mode 100644 app/views/cms/form_fields/new.html.erb delete mode 100644 app/views/cms/form_fields/preview.html.erb delete mode 100644 app/views/cms/forms/_form.html.erb delete mode 100644 app/views/cms/forms/render.html.erb delete mode 100644 app/views/cms/forms/show.html.erb delete mode 100644 features/content_blocks/forms.feature delete mode 100644 spec/cms/form_entry_spec.rb delete mode 100644 spec/cms/form_fields_spec.rb delete mode 100644 spec/cms/form_spec.rb delete mode 100644 test/functional/cms/form_entries_controller_test.rb delete mode 100644 test/functional/cms/form_fields_controller_test.rb delete mode 100644 test/functional/cms/forms_controller_test.rb diff --git a/app/assets/javascripts/cms/form_builder.js b/app/assets/javascripts/cms/form_builder.js deleted file mode 100644 index 02e529164..000000000 --- a/app/assets/javascripts/cms/form_builder.js +++ /dev/null @@ -1,250 +0,0 @@ -//= require cms/ajax -//= require underscore -//= require jquery.exists - -/** - * The UI for dynamically creating custom forms via the UI. - * @constructor - * - */ - -var FormBuilder = function() { -}; - -// Add a new field to the form -// (Implementation: Clone existing hidden form elements rather than build new ones via HTML). -FormBuilder.prototype.newField = function(field_type) { - this.hideNewFormInstruction(); - this.addPreviewFieldToForm(field_type); - -}; - -FormBuilder.prototype.addPreviewFieldToForm = function(field_type) { - $("#placeHolder").load($('#placeHolder').data('new-path') + '?field_type=' + field_type + ' .control-group', function() { - var newField = $("#placeHolder").find('.control-group'); - newField.insertBefore('#placeHolder'); - formBuilder.enableFieldButtons(); - formBuilder.resetAddFieldButton(); - }); -}; - -FormBuilder.prototype.resetAddFieldButton = function() { - $("#form_new_entry_new_field").val('1'); -}; - -FormBuilder.prototype.removeCurrentField = function() { - this.field_being_editted.remove(); - this.field_being_editted = null; -}; - -// Function that triggers when users click the 'Delete' field button. -FormBuilder.prototype.confirmDeleteFormField = function() { - formBuilder.field_being_editted = $(this).parents('.control-group'); - - var path = $(this).attr('data-path'); - if (path == "") { - formBuilder.removeCurrentField(); - } else { - $('#modal-confirm-delete-field').modal({ - show: true - }); - } -}; - -// Function that triggers when users click the 'Edit' field button. -FormBuilder.prototype.editFormField = function() { - // This is the overall container for the entire field. - formBuilder.field_being_editted = $(this).parents('.control-group'); - $('#modal-edit-field').modal({ - show: true, - remote: $(this).attr('data-edit-path') - }); - -}; - - -FormBuilder.prototype.hideNewFormInstruction = function() { - var no_fields = $("#no-field-instructions"); - if (no_fields.exists()) { - no_fields.hide(); - } -}; - -// Add handler to any edit field buttons. -FormBuilder.prototype.enableFieldButtons = function() { - $('.edit_form_button').unbind('click').on('click', formBuilder.editFormField); - $('.delete_field_button').unbind('click').on('click', formBuilder.confirmDeleteFormField); -}; - -FormBuilder.prototype.newFormField = function() { - return $('#ajax_form_field'); -}; - -// Delete field from form, then remove it from the field -FormBuilder.prototype.deleteFormField = function() { - var element = formBuilder.field_being_editted.find('.delete_field_button'); - var url = element.attr('data-path'); - $.cms_ajax.delete({ - url: url, - success: function(field) { - formBuilder.removeCurrentField(); - formBuilder.removeFieldId(field.id); - } - }); -}; - -// @param [Number] value The id of the field that is to be removed from the form. -FormBuilder.prototype.removeFieldId = function(value) { - var field_ids = $('#field_ids').val().split(" "); - field_ids.splice($.inArray(value.toString(), field_ids), 1); - formBuilder.setFieldIds(field_ids); -}; - -// @param [Array] value -FormBuilder.prototype.setFieldIds = function(value) { - var spaced_string = value.join(" "); - $('#field_ids').val(spaced_string); -}; - -FormBuilder.prototype.addFieldIdToList = function(new_value) { - $('#field_ids').val($('#field_ids').val() + " " + new_value); -}; - -// Save a new Field to the database for the current form. -FormBuilder.prototype.createField = function() { - var form = formBuilder.newFormField(); - var data = form.serialize(); - var url = form.attr('action'); - - $.ajax({ - type: "POST", - url: url, - data: data, - global: false, - datatype: $.cms_ajax.asJSON() - }).done( - function(field) { - formBuilder.clearFieldErrorsOnCurrentField(); - - formBuilder.addFieldIdToList(field.id); - formBuilder.field_being_editted.find('input').attr('data-id', field.id); - formBuilder.field_being_editted.find('label').html(field.label); - formBuilder.field_being_editted.find('a').attr('data-edit-path', field.edit_path); - formBuilder.field_being_editted.find('a.delete_field_button').attr('data-path', field.delete_path); - formBuilder.field_being_editted.find('.help-block').html(field.instructions); - - } - ).fail(function(xhr, textStatus, errorThrown) { - formBuilder.displayErrorOnField(formBuilder.field_being_editted, xhr.responseJSON); - }); - -}; - -FormBuilder.prototype.clearFieldErrorsOnCurrentField = function() { - var field = formBuilder.field_being_editted; - field.removeClass("error"); - field.find('.help-inline').remove(); -}; - -FormBuilder.prototype.displayErrorOnField = function(field, json) { - var error_message = json.errors[0]; -// console.log(error_message); - field.addClass("error"); - var input_field = field.find('.input-append'); - input_field.after('' + error_message + ''); -}; - -// Edit Field should handle Enter by submitting the form via AJAX. - // Enter within textareas should still add endlines as normal. -FormBuilder.prototype.onEnterSubmitFormViaAjax = function() { - this.newFormField().on("keypress", function(e) { - if (e.which == 13 && e.target.tagName != 'TEXTAREA') { - formBuilder.createField(); - e.preventDefault(); - $('#modal-edit-field').modal('hide'); - return false; - } - }); -}; -// Attaches behavior to the proper element. -FormBuilder.prototype.setup = function() { - var select_box = $('.add-new-field'); - if (select_box.exists()) { - select_box.change(function() { - formBuilder.newField($(this).val()); - }); - - this.enableFieldButtons(); - $("#delete_field").on('click', formBuilder.deleteFormField); - - $('#modal-edit-field').on('hidden.bs.modal', function(e) { - $(this).removeData('bs.modal'); - }); - - // Allow fields to be sorted. - $('#form-preview').sortable({ - axis: 'y', - delay: 250, - - // When form element is moved - update: function(event, ui) { - var field_id = ui.item.find('input').attr('data-id'); - var new_position = ui.item.index() + 1; - formBuilder.moveFieldTo(field_id, new_position); - } - }); - this.setupConfirmationBehavior(); - this.enableFormCleanup(); - } -}; - -// Since we create a form for the #new action, we need to delete it if the user doesn't save it explicitly. -FormBuilder.prototype.enableFormCleanup = function() { - var cleanup_element = $('#cleanup-before-abandoning'); - if (cleanup_element.exists()) { - var cleanup_on_leave = true; - $(":submit").on('click', function() { - cleanup_on_leave = false; - }); - $(window).bind('beforeunload', function() { - if (cleanup_on_leave) { - var path = cleanup_element.attr('data-path'); - $.cms_ajax.delete({url: path, async: false}); - } - }); - } -}; - -// Updates the server with the new position for a given field. -FormBuilder.prototype.moveFieldTo = function(field_id, position) { - var url = '/cms/form_fields/' + field_id + '/insert_at/' + position; - - var success = function(data) { - console.log("Success:", data); - }; - console.log('For', field_id, 'to', position); - $.post(url, success); -}; - -FormBuilder.prototype.setupConfirmationBehavior = function() { - // Confirmation Behavior - $("#form_confirmation_behavior_show_text").on('click', function() { - $(".form_confirmation_text").show(); - $(".form_confirmation_redirect").hide(); - }); - $("#form_confirmation_behavior_redirect").on('click', function() { - $(".form_confirmation_redirect").show(); - $(".form_confirmation_text").hide(); - }); - $("#form_confirmation_behavior_show_text").trigger('click'); -}; -var formBuilder = new FormBuilder(); - -// Register FormBuilder handlers on page load. -jQuery(function($){ - formBuilder.setup(); - - - // Include a text field to start (For easier testing) -// formBuilder.newField('text_field'); -}); diff --git a/app/assets/stylesheets/cms/default-forms.scss b/app/assets/stylesheets/cms/default-forms.scss deleted file mode 100644 index 133efb605..000000000 --- a/app/assets/stylesheets/cms/default-forms.scss +++ /dev/null @@ -1,3 +0,0 @@ -// Default styles for public CMS Forms (built via the forms module. - -@import "bootstrap"; diff --git a/app/controllers/cms/form_entries_controller.rb b/app/controllers/cms/form_entries_controller.rb deleted file mode 100644 index a73000f77..000000000 --- a/app/controllers/cms/form_entries_controller.rb +++ /dev/null @@ -1,140 +0,0 @@ -module Cms - class FormEntriesController < Cms::BaseController - - include ContentRenderingSupport - - helper_method :content_type - helper Cms::ContentBlockHelper - - allow_guests_to [:submit] - - # Handles public submission of a form. - def submit - find_form_and_populate_entry - if @entry.save - if @form.show_text? - show_content_as_page(@form) - render layout: Cms::Form.layout - else - redirect_to @form.confirmation_redirect - end - unless @form.notification_email.blank? - Cms::EmailMessage.create!( - :recipients => @form.notification_email, - :subject => "[CMS Form] A new entry has been created", - :body => "A visitor has filled out the #{@form.name} form. The entry can be found here: - #{Cms::EmailMessage.absolute_cms_url(cms.form_entry_path(@entry)) }" - ) - end - else - show_content_as_page(@form) - render 'error', layout: Cms::Form.layout - end - end - - def bulk_update - # Duplicates ContentBlockController#bulk_update - ids = params[:content_id] || [] - models = ids.collect do |id| - FormEntry.find(id.to_i) - end - - if params[:commit] == 'Delete' - deleted = models.select do |m| - m.destroy - end - flash[:notice] = "Deleted #{deleted.size} records." - end - - redirect_to entries_path(params[:form_id]) - end - - # Same behavior as ContentBlockController#index - def index - @form = Cms::Form.where(id: params[:id]).first - - # Allows us to use the content_block/index view - @content_type = FauxContentType.new(@form) - @search_filter = SearchFilter.build(params[:search_filter], Cms::FormEntry) - - @blocks = Cms::FormEntry.where(form_id: params[:id]).search(@search_filter.term).paginate({page: params[:page], order: params[:order]}) - @entry = Cms::FormEntry.for(@form) - - @total_number_of_items = @blocks.size - - end - - def edit - @entry = Cms::FormEntry.find(params[:id]) - end - - def update - @entry = Cms::FormEntry.find(params[:id]).enable_validations - if @entry.update(entry_params(@entry)) - redirect_to form_entry_path(@entry) - else - render :edit - end - end - - def show - @entry = Cms::FormEntry.find(params[:id]) - end - - def new - @entry = Cms::FormEntry.for(Form.find(params[:form_id])) - end - - def create - find_form_and_populate_entry - if @entry.save - redirect_to entries_path(@form) - else - save_entry_failure - end - end - - def save_entry_failure - render :new - end - - protected - - def find_form_and_populate_entry - @form = Cms::Form.find(params[:form_id]) - @entry = Cms::FormEntry.for(@form) - @entry.attributes = entry_params(@entry) - end - - def entry_params(entry) - params.require(:form_entry).permit(entry.permitted_params) - end - - # Allows Entries to be displayed using same view as Content Blocks. - class FauxContentType < Cms::ContentType - def initialize(form) - @form = form - self.name = 'Cms::FormEntry' - end - - def display_name - 'Entry' - end - - def display_name_plural - "Entries for #{@form.name} form" - end - def columns_for_index - cols = @form.fields.collect do |field| - {:label => field.label, :method => field.name} - end - cols - end - end - - def content_type - @content_type - end - - end -end \ No newline at end of file diff --git a/app/controllers/cms/form_fields_controller.rb b/app/controllers/cms/form_fields_controller.rb deleted file mode 100644 index 3c88f41ba..000000000 --- a/app/controllers/cms/form_fields_controller.rb +++ /dev/null @@ -1,74 +0,0 @@ -module Cms - class FormFieldsController < Cms::BaseController - - layout false - - def new - @field = Cms::FormField.new(label: 'Untitled', field_type: params[:field_type], form_id: params[:form_id]) - end - - def preview - @form = Cms::Form.find(params[:id]) - @field = Cms::FormField.new(label: 'Untitled', name: :untitled, field_type: params[:field_type], form: @form) - end - - def create - form = Cms::Form.find(params[:form_field].delete(:form_id)) - field = FormField.new(form_field_params) - field.form = form - if field.save - include_edit_path_in_json(field) - include_delete_path_in_json(field) - render json: field - else - render json: { - errors: field.errors.full_messages - }, - success: false, - status: :unprocessable_entity - end - end - - def edit - @field = FormField.find(params[:id]) - render :new - end - - def update - field = FormField.find(params[:id]) - if field.update form_field_params - include_edit_path_in_json(field) - render json: field - else - render plain: "Fail", status: 500 - end - end - - def destroy - field = FormField.find(params[:id]) - field.destroy - render json: field, success: true - end - - def insert_at - field = FormField.find(params[:id]) - field.insert_at(params[:position]) - render json: field - end - - protected - - # For UI to update for subsequent editing. - def include_edit_path_in_json(field) - field.edit_path = cms.edit_form_field_path(field) - end - - def include_delete_path_in_json(field) - field.delete_path = cms.form_field_path(field) - end - - def form_field_params() - params.require(:form_field).permit(FormField.permitted_params) - end - end -end \ No newline at end of file diff --git a/app/controllers/cms/forms_controller.rb b/app/controllers/cms/forms_controller.rb deleted file mode 100644 index 6aaf3af9c..000000000 --- a/app/controllers/cms/forms_controller.rb +++ /dev/null @@ -1,35 +0,0 @@ -class Cms::FormsController < Cms::ContentBlockController - - before_action :associate_form_fields, only: [:create, :update] - before_action :strip_new_entry_params, only: [:create, :update] - - helper do - # For new forms, if the user doesn't complete and save them, we need to delete them from the database. - # The reason :new creates a form object (which is not conventional) is to allow AJAX FormField creation/association. - def cleanup_before_abandoning - ["new", "create"].include? action_name - end - end - - def new - super - @block.confirmation_text = "Thanks for filling out this form." - @block.save! - end - - protected - - # Split the space separated list of ids into an actual array of ids. - # Rails might have a more conventional way to do this, but I couldn't figure it out.' - def associate_form_fields - field_ids = params[:field_ids].split(" ") - params[:form][:field_ids] = field_ids - end - - - # params[:form][:new_entry] is just a garbage parameter that exists to make displaying forms work. - # We want to ignore anything submitted here - def strip_new_entry_params - params[:form].delete(:new_entry) - end -end diff --git a/app/models/cms/form.rb b/app/models/cms/form.rb deleted file mode 100644 index 76642e6a3..000000000 --- a/app/models/cms/form.rb +++ /dev/null @@ -1,47 +0,0 @@ -module Cms - class Form < ActiveRecord::Base - acts_as_content_block - content_module :forms - # is_addressable path: '/forms' - - has_many :fields, -> {order(:position)}, class_name: 'Cms::FormField' - has_many :entries, class_name: 'Cms::FormEntry' - - validates_format_of :notification_email, :with => /@/, unless: lambda { |form| form.notification_email.blank? } - - # Copy any field related errors into the model :base so they get displayed at the top of the form. - after_validation do - unless errors[:fields].empty? - fields.each do |field| - field.errors.each do |attribute, error| - errors[:base] << field.errors.full_message(attribute, error) - end - - end - end - end - - def field(name) - fields.select {|f| f.name == name}.first - end - - def required?(name) - field = field(name) - field ? field.required? : false - end - - def field_names - fields.collect { |f| f.name } - end - - def show_text? - confirmation_behavior.to_sym == :show_text - end - - # Provides a sample Entry for the form. - # This allows us to use SimpleForm to layout out elements but ignore the input when the form submits. - def new_entry - Cms::Entry.new(form: self) - end - end -end diff --git a/app/models/cms/form_entry.rb b/app/models/cms/form_entry.rb deleted file mode 100644 index 8869f0d02..000000000 --- a/app/models/cms/form_entry.rb +++ /dev/null @@ -1,75 +0,0 @@ -module Cms - class FormEntry < ActiveRecord::Base - - store :data_columns - belongs_to :form, class_name: 'Cms::Form', required: false - - after_initialize :add_field_accessors - - def permitted_params - form.field_names - end - - # Returns a copy of the persisted object. This is required so that existing records fetched from the db can - # have validation during update operations. - # - # @return [Cms::FormEntry] A copy of this record with validations enabled on it. - def enable_validations - entry = FormEntry.for(form) - entry.attributes = self.attributes - entry.instance_variable_set(:@new_record, false) - entry - end - - class << self - - def search(term) - where("data_columns like ?", "%#{term}%") - end - - # Create an Entry for a specific Form. It will have validation and accessors based on the fields of the form. - # - # @param [Cms::Form] form - def for(form) - entry = FormEntry.ish(form: form) { - form.field_names.each do |field_name| - if form.required?(field_name) - validates field_name, presence: true - end - end - } - entry - end - - # Create an instance of a FormEntry with the given methods. - def ish(*args, &block) - dup(&block).new(*args) - end - - # Creates a faux class with singleton methods. Solves this problem: - # https://github.com/rails/rails/issues/5449 - def dup(&block) - super.tap do |dup| - def dup.name() - FormEntry.name - end - - dup.class_eval(&block) if block - end - end - end - private - - # Add a single field accessor to the current instance of the object. (I.e. not shared with others) - def add_store_accessor(field_name) - singleton_class.class_eval { store_accessor :data_columns, field_name } - end - - def add_field_accessors - return unless form - form.field_names.each do |field_name| - add_store_accessor(field_name) - end - end - end -end \ No newline at end of file diff --git a/app/models/cms/form_field.rb b/app/models/cms/form_field.rb deleted file mode 100644 index 2b2dc7683..000000000 --- a/app/models/cms/form_field.rb +++ /dev/null @@ -1,78 +0,0 @@ -module Cms - class FormField < ActiveRecord::Base - extend DefaultAccessible - - belongs_to :form, required: false - acts_as_list scope: :form - - attr_accessor :edit_path, :delete_path - - # Name is assigned when the Field is created, and should not be changed afterwords. - # Otherwise existing entries will not display their data correctly. - # - # FormField#name is used for input fields. I.e. <%= f.input field.name %> - before_validation(on: :create) do - self.name = label.parameterize.underscore.to_sym if label - end - - validates :name, :uniqueness => {:scope => :form_id, message: "can only be used once per form."} - - def as_json(options={}) - super(:methods => [:edit_path, :delete_path]) - end - - - # Return the form widget that should be used to render this field. - # - # @return [Symbol] A SimpleForm input mapping (i.e. :string, :text) - def as - case field_type - when "text_field" - :string - when "text_area" - :text - else - field_type.to_sym - end - end - - # Return options to be passed to a SimpleForm input. - # @return [Hash] - # @param [Hash] config - # @option config [Boolean] :disabled If the field should be disabled. - # @option config [FormEntry] :entry - def options(config={}) - opts = {label: label} - if field_type != "text_field" - opts[:as] = self.as - end - if config[:disabled] - opts[:disabled] = true - opts[:readonly] = 'readonly' - end - opts[:required] = !!required - opts[:hint] = instructions if instructions - unless choices.blank? - opts[:collection] = parse_choices - opts[:prompt] = !self.required? - end - unless config[:entry] && config[:entry].send(name) - opts[:input_html] = {value: default_value} - end - opts - end - - - # Don't allow name to be set via UI. - def self.permitted_params - super - [:name] - end - - private - - # Choices is a single text area where choices are divided by endlines. - def parse_choices - choices.split( /\r?\n/ ) - end - end -end \ No newline at end of file diff --git a/app/views/cms/form_entries/_buttons.html.erb b/app/views/cms/form_entries/_buttons.html.erb deleted file mode 100644 index 4d9fdbb98..000000000 --- a/app/views/cms/form_entries/_buttons.html.erb +++ /dev/null @@ -1,2 +0,0 @@ -<%= link_to("#{@entry.form.name} form", form_path(@entry.form), class: "btn btn-small right ") %> -<%= link_to("New Entry", new_form_entry_path(form_id: @entry.form), class: "btn btn-small btn-primary right") %> \ No newline at end of file diff --git a/app/views/cms/form_entries/_form.html.erb b/app/views/cms/form_entries/_form.html.erb deleted file mode 100644 index 797d46970..000000000 --- a/app/views/cms/form_entries/_form.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= simple_form_for(model, as: :form_entry, url: url) do |f| %> - <%= hidden_field_tag :form_id, model.form.id %> - <% model.form.fields.each do |field| %> - <%= f.input field.name, field.options(entry: model) %> - <% end %> - <%= f.button :submit, "Submit" %> -<% end %> \ No newline at end of file diff --git a/app/views/cms/form_entries/_internal_form.html.erb b/app/views/cms/form_entries/_internal_form.html.erb deleted file mode 100644 index 57251f456..000000000 --- a/app/views/cms/form_entries/_internal_form.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -<% content_for :buttons, 'save_buttons' %> -<%= simple_form_for(model, as: :form_entry, url: url) do |f| %> - <%= render layout: 'form_with_buttons', locals: {f: f} do %> - <%= hidden_field_tag :form_id, model.form.id %> - <% model.form.fields.each do |field| %> - <%= f.input field.name, field.options(entry: model) %> - <% end %> - <% end %> -<% end %> \ No newline at end of file diff --git a/app/views/cms/form_entries/edit.html.erb b/app/views/cms/form_entries/edit.html.erb deleted file mode 100644 index db16de9db..000000000 --- a/app/views/cms/form_entries/edit.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% use_page_title "Edit Entry" %> -<%= render partial: 'internal_form', - locals: {model: @entry, - url: cms.form_entry_path(@entry)} -%> diff --git a/app/views/cms/form_entries/error.html.erb b/app/views/cms/form_entries/error.html.erb deleted file mode 100644 index 3af4a2aa8..000000000 --- a/app/views/cms/form_entries/error.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= content_for :main do %> - <%= render file: 'cms/forms/render' %> -<% end %> \ No newline at end of file diff --git a/app/views/cms/form_entries/index.html.erb b/app/views/cms/form_entries/index.html.erb deleted file mode 100644 index 1e7d5c6bf..000000000 --- a/app/views/cms/form_entries/index.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% content_for :bulk_actions do %> - <%= render 'buttons' %> - <%= hidden_field_tag :form_id, @form.id %> -<% end %> -<%= render file: 'cms/content_block/index' %> diff --git a/app/views/cms/form_entries/new.html.erb b/app/views/cms/form_entries/new.html.erb deleted file mode 100644 index 1bdb47efa..000000000 --- a/app/views/cms/form_entries/new.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% use_page_title "New Entry for #{@entry.form.name}" %> -<%= render partial: 'internal_form', - locals: {model: @entry, - url: form_entries_path(@entry)} -%> diff --git a/app/views/cms/form_entries/show.html.erb b/app/views/cms/form_entries/show.html.erb deleted file mode 100644 index a315b4a86..000000000 --- a/app/views/cms/form_entries/show.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% use_page_title "View Entry" %> - -<%= render partial: 'buttons', layout: 'page_title' %> - - -<%= render layout: 'main_with_sidebar' do %> -
- <% @entry.form.fields.each do |field| %> -
<%= field.label %>
-
<%= @entry.data_columns[field.name] %>
- <% end %> -
-<% end %> \ No newline at end of file diff --git a/app/views/cms/form_entries/submit.html.erb b/app/views/cms/form_entries/submit.html.erb deleted file mode 100644 index d00f3bbbd..000000000 --- a/app/views/cms/form_entries/submit.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= content_for :main , @form.confirmation_text.html_safe %> \ No newline at end of file diff --git a/app/views/cms/form_fields/_form.html.erb b/app/views/cms/form_fields/_form.html.erb deleted file mode 100644 index e6c760a3b..000000000 --- a/app/views/cms/form_fields/_form.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -<%= f.input field.name, - label: field.label, - hint: field.instructions, - wrapper: :append do %> - <%= f.input_field field.name, field.options(disabled: true).merge({data: {id: field.id}}) %> - <%= link_to("Edit", "#", data: {edit_path: edit_path}, class: "btn edit_form_button") %> - <%= link_to("Delete", "#", data: {path: delete_path}, class: "btn delete_field_button") %> -<% end %> diff --git a/app/views/cms/form_fields/_select.html.erb b/app/views/cms/form_fields/_select.html.erb deleted file mode 100644 index a11012312..000000000 --- a/app/views/cms/form_fields/_select.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= f.input :choices, hint: "Enter each choice on a separate line." %> -<%= f.input :required, hint: "Required fields will show the first option a preselected." %> -<%= f.input :instructions, as: :text %> diff --git a/app/views/cms/form_fields/_text_area.html.erb b/app/views/cms/form_fields/_text_area.html.erb deleted file mode 100644 index 5a7444617..000000000 --- a/app/views/cms/form_fields/_text_area.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= f.input :required %> -<%= f.input :instructions, as: :text %> -<%= f.input :default_value %> \ No newline at end of file diff --git a/app/views/cms/form_fields/_text_field.html.erb b/app/views/cms/form_fields/_text_field.html.erb deleted file mode 100644 index 5a7444617..000000000 --- a/app/views/cms/form_fields/_text_field.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= f.input :required %> -<%= f.input :instructions, as: :text %> -<%= f.input :default_value %> \ No newline at end of file diff --git a/app/views/cms/form_fields/edit.html.erb b/app/views/cms/form_fields/edit.html.erb deleted file mode 100644 index e69de29bb..000000000 diff --git a/app/views/cms/form_fields/new.html.erb b/app/views/cms/form_fields/new.html.erb deleted file mode 100644 index ac0edd62c..000000000 --- a/app/views/cms/form_fields/new.html.erb +++ /dev/null @@ -1,26 +0,0 @@ -
- -

Edit Field

-
-
- <%= simple_form_for(@field, html: {id: 'ajax_form_field'}) do |f| %> - <%= f.hidden_field :form_id %> - <%= f.hidden_field :field_type %> - <%= f.input :label, hint: @field.new_record? ? "" : "Will not change the database name (i.e. '#{@field.name}') for new or existing entries. ", autofocus: true %> - <%= render partial: @field.field_type, locals: {f: f, field: @field} %> - <%= f.button :submit, "Save", style: 'display: none' %> - <% end %> -
-
- Save - -
- -<%# - I would prefer to do this on shown.bs.modal event but that doesn't seem to work. - Doing it here guarantees the #create_field exists before we add the click to it. - %> - diff --git a/app/views/cms/form_fields/preview.html.erb b/app/views/cms/form_fields/preview.html.erb deleted file mode 100644 index 2401f7fcd..000000000 --- a/app/views/cms/form_fields/preview.html.erb +++ /dev/null @@ -1,16 +0,0 @@ -<%= simple_form_for(@form) do |f| %> - <%= f.simple_fields_for :new_entry do |e| %> - <% - locals = {f: e, - field: @field, - edit_path: new_form_field_path(form_id: @form.id, field_type: @field.field_type) - } - if @field.persisted? - locals[:delete_path] = form_field_path(@field.id) - else - locals[:delete_path] = "" - end - %> - <%= render partial: 'cms/form_fields/form', locals: locals %> - <% end %> -<% end %> \ No newline at end of file diff --git a/app/views/cms/forms/_form.html.erb b/app/views/cms/forms/_form.html.erb deleted file mode 100644 index e9d1ca1ef..000000000 --- a/app/views/cms/forms/_form.html.erb +++ /dev/null @@ -1,67 +0,0 @@ -<% if cleanup_before_abandoning %> - -<% end %> - -

Form Settings

-<%= f.input :name, as: :name %> -<%= f.input :slug, as: :path %> -<%= f.input :description %> -<%= hidden_field_tag "field_ids", @block.field_ids %> -<%= f.input :confirmation_behavior, label: "On Confirmation", - collection: [["Show Text", :show_text], ['Redirect to Website', :redirect]], - as: :radio_buttons %> -<%= f.input :confirmation_text, label: false %> -<%= f.input :confirmation_redirect, label: false, hint: "A complete URL (i.e. http://www.yoursite.com/registration) or relative path (/thanks-for-registering)." %> -<%= f.input :notification_email, placeholder: 'email@example.com', label: "Notification Email", hint: "Notify this email address when an entry is created. (Leave blank for no notification)" %> - -

Preview

-
- <% if @block.new_record? %> -
- No fields yet! This live preview of your form will show you fields as you add them. Choose a field type - below. -
- <% end %> - <%= f.simple_fields_for :new_entry do |e| %> - <% @block.fields.each do |field| %> - <%= render partial: 'cms/form_fields/form', - locals: {f: e, field: field, edit_path: edit_form_field_path(field.id), delete_path: form_field_path(field.id)} %> - <% end %> - <% end %> - -
- -<%= f.simple_fields_for :new_entry do |e| %> - <%= e.input :new_field, - label: "Add New Field", - prompt: "Select a field type:", - collection: [["Text Field", :text_field], ["Text Box", :text_area], ["Dropdown", :select]], - input_html: {class: 'add-new-field'} - %> -<% end %> -<%# Can't have a form inside a form, so this must go into the head. %> -<%= content_for :html_head do %> - - - -<% end %> - -<% content_for :sidebar_actions do %> -
- <%= link_to "Entries", cms.entries_path(@block), class: "btn btn-small" %> -
-<% end %> diff --git a/app/views/cms/forms/render.html.erb b/app/views/cms/forms/render.html.erb deleted file mode 100644 index e07f74b37..000000000 --- a/app/views/cms/forms/render.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% content_for :html_head do %> - <%= stylesheet_link_tag Rails.application.config.cms.form_builder_css %> -<% end %> -

<%= @content_block.name %>

-

- <%= @content_block.description %> -

-<% - unless @entry - @entry = Cms::FormEntry.for(@content_block) - end -%> -<%= render partial: 'cms/form_entries/form', locals: {model: @entry, url: cms.submit_form_entries_path, form: @content_block} %> - - diff --git a/app/views/cms/forms/show.html.erb b/app/views/cms/forms/show.html.erb deleted file mode 100644 index f96fc04b9..000000000 --- a/app/views/cms/forms/show.html.erb +++ /dev/null @@ -1,6 +0,0 @@ -<% content_for :page_buttons do %> -
- <%= link_to("Entries", cms.entries_path(@block), class: "btn btn-small") %> -
-<% end %> -<%= render file: 'cms/content_block/show' %> \ No newline at end of file diff --git a/features/content_blocks/forms.feature b/features/content_blocks/forms.feature deleted file mode 100644 index 81740fe94..000000000 --- a/features/content_blocks/forms.feature +++ /dev/null @@ -1,28 +0,0 @@ -Feature: Forms - - Editors should be able to create and manage forms to collect information from site visitors. They should be able - to build these forms dynamically through the UI. - - Background: - Given I am logged in as a Content Editor - -# Forms a broken -# Scenario: List Forms -# Given I had created a form named "Contact Us" -# When I select forms from the content library -# Then I should see the list of forms -# And I should see the "Contact Us" form in the list -# -# Scenario: Add Form -# When I am adding new form -# And I enter the required form fields -# Then after saving I should be redirect to the form page -# -# Scenario: Edit Form -# Given I had created a form named "Contact Us" -# When I edit that form -# And I make changes to the form -# Then I should see the form with the updated fields - - - diff --git a/spec/cms/form_entry_spec.rb b/spec/cms/form_entry_spec.rb deleted file mode 100644 index e7f2db907..000000000 --- a/spec/cms/form_entry_spec.rb +++ /dev/null @@ -1,153 +0,0 @@ -require "minitest_helper" - -def expect_nonrequired_fields(form) - mock_field = mock() - mock_field.expects(:required?).returns(false).at_least_once - form.expects(:field).returns(mock_field).at_least_once -end - -describe Cms::FormEntry do - let(:entry) { Cms::FormEntry.new } - - def contact_form - return @form if @form - @form = Cms::Form.new - @form.expects(:field_names).returns([:name, :email]).at_least_once - @form - end - - let(:contact_form_entry) { Cms::FormEntry.for(contact_form) } - - describe '.ish' do - it "should create object with class_eval'd methods'" do - entry = Cms::FormEntry.ish { - def hello - end - } - entry.respond_to?(:hello).must_equal true - end - end - - describe '.new' do - it "should create a entry with no accessors" do - entry.wont_be_nil - end - end - - def form_with_fields(fields) - form = Cms::Form.new - fields.each do |f| - form.fields << Cms::FormField.create!(f) - end - form.save! - form - end - - describe '#valid?' do - it "should return false if required fields are not filled in" do - form = form_with_fields([{label: "Name", required: true}]) - - form_entry = Cms::FormEntry.for(form) - form_entry.valid?.must_equal false - end - end - - describe '#data_columns' do - it 'should return nil for unset attributes' do - entry.data_columns['name'].must_be_nil - end - - it 'should allow arbitrary access' do - entry.data_columns['name'] = 'Stan' - entry.save! - entry.reload.data_columns['name'].must_equal 'Stan' - end - - it "should not share accessors across forms" do - [:name, :email].each do |field| - contact_form_entry.respond_to?(field).must_equal true - end - registration_form = Cms::Form.new - registration_form.expects(:field_names).returns([:first_name, :last_name]).at_least_once - expect_nonrequired_fields(registration_form) - registration_entry = Cms::FormEntry.for(registration_form) - - [:first_name, :last_name].each do |field| - registration_entry.respond_to?(field).must_equal true - end - [:name, :email].each do |field| - registration_entry.respond_to?(field).must_equal false - end - end - - it "should coerce fields to their proper type" do - entry.data_columns[:age] = 1 - entry.save! - - entry.reload.data_columns[:age].must_be_instance_of Fixnum - end - end - - describe '#prepare' do - it "should return a copy of a FormEntry with the same data and accessors" do - form = Cms::Form.create!(name: "Contact") - form.fields << Cms::FormField.create!(label: 'Name', required: true) - form.save! - - entry = Cms::FormEntry.for(form) - entry.name = "Filled in" - entry.save! - - e = entry.enable_validations - e.id.must_equal entry.id - e.new_record?.must_equal false - e.name.must_equal "Filled in" - e.name = "" - e.valid?.must_equal false - end - end - - describe '.for' do - it "should create FormEntry with accessors for backing form" do - form = Cms::Form.new - form.expects(:field_names).returns([:name, :email]).at_least_once - entry = Cms::FormEntry.for(form) - - entry.name = "Hello" - entry.name.must_equal 'Hello' - entry.data_columns[:name].must_equal 'Hello' - entry.respond_to? :name - entry.respond_to? :email - end - - it "should connect form to entry" do - form = Cms::Form.new - entry = Cms::FormEntry.for(form) - - entry.form.must_equal form - end - end - - describe '#form' do - - it "should be a belongs_to association" do - form = Cms::Form.create!(name: "Contact") - contact_form_entry = Cms::FormEntry.new(form: form) - contact_form_entry.save! - - contact_form_entry = Cms::FormEntry.find(contact_form_entry.id) - contact_form_entry.form.must_equal form - end - - end - - describe '#permitted_params' do - it "should respond" do - contact_form_entry.respond_to?(:name).must_equal true - end - - it "should return all the fields for the specific form" do - contact_form_entry.permitted_params.must_equal [:name, :email] - end - end -end diff --git a/spec/cms/form_fields_spec.rb b/spec/cms/form_fields_spec.rb deleted file mode 100644 index 0b5198747..000000000 --- a/spec/cms/form_fields_spec.rb +++ /dev/null @@ -1,143 +0,0 @@ -require "minitest_helper" - -describe Cms::FormField do - - describe '.permitted_params' do - it 'should return an array of fields' do - [:form_id, :label, :field_type, :required, :position, :instructions, :default_value].each do |field| - Cms::FormField.permitted_params.must_include field - end - end - end - - describe '#form' do - it "should be belongs_to" do - form = Cms::Form.create!(name: "Testing") - field = Cms::FormField.new(label: "Name") - field.form = form - field.save! - field.form.wont_be_nil - - field.form_id.must_equal field.form.id - end - - it "should trigger validation with duplicate names" do - form = Cms::Form.create!(name: "Testing") - field = Cms::FormField.new(label: "Name", form_id: form.id) - field.save.must_equal true - - duplicate_field = Cms::FormField.new(label: "Name", form_id: form.id) - duplicate_field.save.must_equal false - end - end - - describe '#name' do - it "should return a symbol that can be used as the name for inputs" do - field = Cms::FormField.create!(label: 'Name') - field.name.must_equal 'name' - end - - it "should underscore names" do - field = Cms::FormField.create!(label: 'Full Name') - field.name.must_equal 'full_name' - end - - it "should not change after being saved even when the label is changed" do - field = Cms::FormField.create!(label: 'Name') - field.update(label: 'Full Name') - field.name.must_equal 'name' - end - - - end - - describe "#options" do - let(:field) { Cms::FormField.new(label: 'Title', field_type: 'text_field') } - - it "can disable the input" do - field.options(disabled: true)[:disabled].must_equal(true) - field.options(disabled: true)[:readonly].must_equal('readonly') - end - it "should provide as: for default cases" do - field.options[:label].must_equal('Title') - end - - it "includes as: for text_areas" do - field.field_type = 'text_area' - field.options[:as].must_equal(:text) - end - - it "should include collection for :select" do - field.choices = "A\nB\nC" - field.options[:collection].must_equal ["A", "B", "C"] - field.options[:prompt].must_equal true - end - - it "set prompt to false for required selects" do - field.choices = "A\nB\nC" - field.required = true - field.options[:collection].must_equal ["A", "B", "C"] - field.options[:prompt].must_equal false - end - - it "should return required false by default" do - field.options[:required].must_equal false - end - it "should handle required fields" do - field.required = true - field.options[:required].must_equal true - end - - it "should return hints" do - field.instructions = "Fill this in" - field.options[:hint].must_equal "Fill this in" - end - - it "should return default value" do - field.default_value = "My Default" - field.options[:input_html][:value].must_equal "My Default" - end - - - it "should not return default value if the model has a value for the field" do - field.valid? # Ensure name is set - field.default_value = "My Default" - entry = mock() - entry.expects(:title).returns("some-value").at_least(0) - field.options({entry: entry}).wont_include(:input_html) - end - - - end - - describe "#as" do - it "should handle text_fields" do - field = Cms::FormField.new(field_type: 'text_field') - field.as.must_equal :string - end - - it "should handle text_areas" do - field = Cms::FormField.new(field_type: 'text_area') - field.as.must_equal :text - end - - it "should handle other random types" do - field = Cms::FormField.new(field_type: 'random') - field.as.must_equal :random - end - - it "should handle select" do - field = Cms::FormField.new(field_type: 'select') - field.as.must_equal :select - end - end - - describe "#as_json" do - let(:field) { Cms::FormField.new(label: 'Name') } - it "should include #edit_path when being serialized" do - field.edit_path = "/cms/form_fields/1/edit" - json = JSON.parse(field.to_json) - json["edit_path"].must_equal "/cms/form_fields/1/edit" - end - end -end diff --git a/spec/cms/form_spec.rb b/spec/cms/form_spec.rb deleted file mode 100644 index ed8f2bc3a..000000000 --- a/spec/cms/form_spec.rb +++ /dev/null @@ -1,134 +0,0 @@ -require "minitest_helper" - -describe Cms::Form do - - let(:form) { Cms::Form.new(name: 'Contact Us') } - describe '.create!' do - it "should create a new instance" do - form.save! - form.name.must_equal "Contact Us" - form.persisted?.must_equal true - end - - it "should create a slug when created " do - skip "Form addressibility removed 6 years ago. app/models/cms/form.rb:5" - form.slug = "/contact-us" - form.save! - form.reload.section_node.wont_be_nil - form.section_node.slug.must_equal "/contact-us" - end - - it "should assign parent with created" do - skip "Parent not getting created (in rails 4?)" - form.save! - form.parent.wont_be_nil - form.section_node.slug.must_equal - end - - end - - describe '#update' do - let(:saved_form) { Cms::Form.create! } - it "should update slug" do - skip "Form addressibility removed 6 years ago. app/models/cms/form.rb:5" - saved_form.update({name: 'New', slug: '/about-us'}).must_equal true - saved_form.reload.section_node.slug.must_equal '/about-us' - end - end - - describe '#show_text?' do - it "should return true with the :show_text confirmation behavior" do - form.confirmation_behavior = :show_text - form.save! - form.reload.show_text?.must_equal true - end - end - - describe "#valid?" do - it "should not allow improperly formatted notification emails (requires @)" do - form.notification_email = "valid@example.com" - form.must_be :valid? - - form.notification_email = "not-valid-example.com" - form.wont_be :valid? - - form.notification_email = "" - form.must_be :valid? - end - end - - describe '#fields' do - it "should save a list of fields" do - field = Cms::FormField.new(label: "Event Name", field_type: :string) - form.fields << field - form.save! - - form.reload.fields.size.must_equal 1 - field.persisted?.must_equal true - end - - it "should set error on :base when there are duplicate fields" do - form.fields << Cms::FormField.create(label: 'Name') - form.fields << Cms::FormField.new(label: 'Name') - form.valid?.must_equal false - form.errors[:base].size.must_equal 1 - form.errors[:base].must_include "Labels can only be used once per form." - end - - describe "ordering" do - def form_with_fields(field_labels=[]) - return @form_with_fields if @form_with_fields - @form_with_fields = Cms::Form.new - field_labels.each do |label| - @form_with_fields.fields << Cms::FormField.create(label: label) - end - @form_with_fields.save! - @form_with_fields - end - - it "should add fields in position order" do - f = form_with_fields(['Name', 'Address']) - f.fields.first.position.must_equal 0 - f.fields.last.position.must_equal 1 - end - - it "are orderable" do - form = form_with_fields(['Name', 'Address']) - form.fields.last.move_to_top - form.reload - form.fields.first.label.must_equal "Address" - end - end - - end - - describe '#field_names' do - - it "should return a list of the field names as symbols" do - form = Cms::Form.new - form.fields << Cms::FormField.new(label: 'Name') - form.fields << Cms::FormField.new(label: 'Email') - form.save! - form.field_names.must_equal ['name', 'email'] - end - end - - describe '#required?' do - - def form_with_name_field - form = Cms::Form.new - form.fields << Cms::FormField.new(label: 'Name', required: true) - form.fields << Cms::FormField.new(label: 'Email') - form.save! - form - end - - it "should return true for required fields" do - form_with_name_field.required?('name').must_equal(true) - end - - it "should returns nil for missing fields" do - form_with_name_field.required?('email').must_equal(false) - end - end -end diff --git a/test/functional/cms/form_entries_controller_test.rb b/test/functional/cms/form_entries_controller_test.rb deleted file mode 100644 index 02ca7745e..000000000 --- a/test/functional/cms/form_entries_controller_test.rb +++ /dev/null @@ -1,176 +0,0 @@ -require 'test_helper' - -# Phase 4, stage F. -# Docs: docs/rails-upgrade/phase-4-implementation-plan.md -# -# Cms::FormEntriesController was at 0% coverage across 108 lines -- the largest -# untested controller in the engine, and the one that handles PUBLIC form submission. -# `allow_guests_to [:submit]` means the submit action is reachable without -# authentication, so it is also the largest untested attack surface. -# -# It is named in Phase 5's manual-verification list for exactly that reason. These -# tests replace part of that manual pass with something that runs every build. -# -# Scope note: this file was written during stage F to close a coverage gate, which -# phase-4-characterization-tests.md otherwise rules out ("writing tests to raise a -# percentage is the wrong objective"). The tests are characterization tests of a -# controller that had none -- the gate was the prompt, not the design. -module Cms - class FormEntriesControllerTest < ActionController::TestCase - tests Cms::FormEntriesController - include Cms::ControllerTestHelper - - def setup - given_a_site_exists - @form = build_form - end - - # Cms::Form is a versioned content block, so `update!` on an existing form creates - # a DRAFT -- and the controller reads the published record, which still has the old - # values. Every attribute a test depends on has to be set at creation time. - def build_form(attrs = {}) - form = create(:form, {name: "Contact Us", confirmation_behavior: 'redirect', - confirmation_redirect: '/thanks'}.merge(attrs)) - form.fields << Cms::FormField.create!(label: "Email", field_type: "text_field") - form.save! - form - end - - def submit_entry(attrs = {email: "visitor@example.com"}) - post :submit, params: {form_id: @form.id, form_entry: attrs} - end - - # --- the public path ----------------------------------------------------- - - test "a guest can submit a form entry without logging in" do - assert_difference 'Cms::FormEntry.count', 1 do - submit_entry - end - - entry = Cms::FormEntry.order(:id).last - assert_equal @form, entry.form - end - - # ------------------------------------------------------------------------- - # CHARACTERIZATION: public form submission is broken for every form configured to - # show confirmation text, which is the default behaviour offered in the UI. - # - # form_entries_controller.rb:17 render layout: Cms::Form.layout (success) - # form_entries_controller.rb:31 render 'error', layout: Cms::Form.layout - # - # `Cms::Form.layout` does not exist -- no model or behavior in the engine defines - # `self.layout`, and Cms::Form.respond_to?(:layout) is false. Both call sites raise - # NoMethodError, so a visitor submitting such a form gets a 500 and the entry's - # confirmation is never shown. The entry IS saved first, so data is not lost. - # - # Fails identically on both bundles -- NOT caused by the Rails upgrade. Found in - # Phase 4 stage F by instantiating a controller that had sat at 0% coverage across - # 108 lines. It is the second defect of this shape in the Forms subsystem; the - # first is Cms::Form.path, characterized in forms_controller_test.rb. - # - # NOT FIXED HERE: the repair is a product decision about which layout a form - # confirmation should render in, not an upgrade one. - # - # This pins the CURRENT behaviour. When it is fixed this test fails -- that is the - # signal to delete it and assert the real confirmation, not to work around it. - # ------------------------------------------------------------------------- - test "CHARACTERIZATION: submit 500s when the form shows confirmation text" do - @form = build_form(confirmation_behavior: 'show_text', - confirmation_text: "Thanks, we got it.") - refute Cms::Form.respond_to?(:layout), - "Cms::Form gained a .layout -- the defect below may be fixed; re-check." - - assert_difference 'Cms::FormEntry.count', 1 do - submit_entry - end - assert_response :internal_server_error, - "expected the known Cms::Form.layout failure. If this now " + - "succeeds, the Forms confirmation path has been repaired." - end - - test "submit redirects when the form is configured to redirect" do - submit_entry - assert_redirected_to '/thanks' - end - - # The notification branch is `unless @form.notification_email.blank?`, so both - # sides need exercising or the blank guard could invert unnoticed and the CMS - # would start mailing on every submission -- or stop mailing entirely. - test "submit sends a notification email when the form has a notification address" do - @form = build_form(notification_email: 'owner@example.com') - - assert_difference 'Cms::EmailMessage.count', 1 do - submit_entry - end - - message = Cms::EmailMessage.order(:id).last - assert_equal 'owner@example.com', message.recipients - assert_match(/Contact Us/, message.body) - end - - test "submit sends no notification email when no address is configured" do - assert_no_difference 'Cms::EmailMessage.count' do - submit_entry - end - end - - # --- the admin paths ----------------------------------------------------- - - test "update saves a valid change to an existing entry" do - submit_entry - entry = Cms::FormEntry.order(:id).last - login_as_cms_admin - - put :update, params: {id: entry.id, form_entry: {email: "changed@example.com"}} - - assert_redirected_to Cms::Engine.routes.url_helpers.form_entry_path(entry) - assert_equal "changed@example.com", entry.reload.email - end - - test "bulk_update deletes the selected entries" do - submit_entry - submit_entry(email: "second@example.com") - login_as_cms_admin - ids = Cms::FormEntry.order(:id).last(2).map(&:id) - - assert_difference 'Cms::FormEntry.count', -2 do - put :bulk_update, params: {content_id: ids.map(&:to_s), commit: 'Delete', form_id: @form.id} - end - assert_equal "Deleted 2 records.", flash[:notice] - end - - # The `params[:content_id] || []` guard. Without it this raises NoMethodError on - # nil rather than doing nothing, and the action is reachable from the admin UI's - # bulk toolbar with nothing selected. - test "bulk_update with nothing selected deletes nothing and does not raise" do - submit_entry - login_as_cms_admin - - assert_no_difference 'Cms::FormEntry.count' do - put :bulk_update, params: {commit: 'Delete', form_id: @form.id} - end - end - - test "bulk_update ignores a commit value other than Delete" do - submit_entry - login_as_cms_admin - ids = [Cms::FormEntry.order(:id).last.id.to_s] - - assert_no_difference 'Cms::FormEntry.count' do - put :bulk_update, params: {content_id: ids, commit: 'Something Else', form_id: @form.id} - end - end - - test "show and edit load the requested entry" do - submit_entry - entry = Cms::FormEntry.order(:id).last - login_as_cms_admin - - get :show, params: {id: entry.id} - assert_equal entry, assigns(:entry) - - get :edit, params: {id: entry.id} - assert_equal entry, assigns(:entry) - end - end -end diff --git a/test/functional/cms/form_fields_controller_test.rb b/test/functional/cms/form_fields_controller_test.rb deleted file mode 100644 index c769754d4..000000000 --- a/test/functional/cms/form_fields_controller_test.rb +++ /dev/null @@ -1,105 +0,0 @@ -require 'test_helper' - -# Phase 4, stage F -- work item 4.4 / Tier B B9, criterion 7. -# Docs: docs/rails-upgrade/phase-4-implementation-plan.md -# -# Cms::FormFieldsController was at 0% coverage: no functional test file existed, so -# nothing ever instantiated it. That matters here because form_fields_controller.rb:16 -# is one of the ten sites that treat an ActionController::Parameters sub-hash as a -# Hash: -# -# form = Cms::Form.find(params[:form_field].delete(:form_id)) -# -# `.delete` does two things at once -- it returns the value AND removes the key -- and -# the code depends on both halves: the returned id finds the Form, and the removal is -# what keeps :form_id out of form_field_params on the next line. -# -# `.delete` still exists on Parameters in Rails 5. What changed is that Parameters is -# no longer a Hash, so surrounding code that assumed Hash semantics can behave -# differently. These tests assert the two halves separately, so a regression in either -# is attributable. -module Cms - class FormFieldsControllerTest < ActionController::TestCase - include Cms::ControllerTestHelper - - def setup - given_a_site_exists - login_as_cms_admin - @form = create(:form, name: "Contact Us") - end - - def create_field(overrides = {}) - post :create, params: { - form_field: { - form_id: @form.id, - label: 'Email Address', - field_type: 'text_field' - }.merge(overrides) - } - end - - test "create associates the field with the form named by the deleted form_id" do - assert_difference 'Cms::FormField.count', 1 do - create_field - end - - assert_response :success - field = Cms::FormField.order(:id).last - assert_equal @form, field.form, - "form_id is pulled out of the params with .delete and used to find " + - "the Form; if that stops working the field is orphaned" - end - - # The other half of the same line. If `.delete` stopped removing the key, :form_id - # would fall through into form_field_params and be mass-assigned -- which happens - # to reach the same result here, so asserting only the association above would not - # notice. This asserts the removal itself. - test "create removes form_id from the params it mass-assigns" do - create_field - assert_response :success - - refute @controller.params[:form_field].key?(:form_id), - "form_field_params is built from params[:form_field] after the delete, so " + - ":form_id must no longer be present" - refute @controller.params[:form_field].key?('form_id') - end - - test "create renders the field as json on success" do - create_field - assert_response :success - - body = JSON.parse(response.body) - assert_equal 'Email Address', body['label'] - assert body.key?('edit_path'), "as_json should include the edit path" - assert body.key?('delete_path'), "as_json should include the delete path" - end - - # The error branch of the same action. Uses the uniqueness validation because it is - # the only one FormField actually has -- there is no presence validation on :label, - # so a blank label is accepted and simply produces a field named :"" (the - # before_validation at form_field.rb:14 does `label.parameterize.underscore.to_sym`). - # Noted rather than fixed: tightening that is a product decision, not an upgrade one. - test "create reports validation errors as json rather than raising" do - create_field - assert_response :success - - assert_no_difference 'Cms::FormField.count' do - create_field # same label, same form -> fails the uniqueness scope - end - assert_response :unprocessable_entity - - body = JSON.parse(response.body) - assert body['errors'].any?, "expected validation errors in the json body" - end - - test "new builds an unsaved field for the requested form and type" do - get :new, params: {form_id: @form.id, field_type: 'text_field'} - - assert_response :success - field = assigns(:field) - assert field.new_record?, "new should not persist the field" - assert_equal @form.id, field.form_id - assert_equal 'text_field', field.field_type - end - end -end diff --git a/test/functional/cms/forms_controller_test.rb b/test/functional/cms/forms_controller_test.rb deleted file mode 100644 index c4ce7e5fc..000000000 --- a/test/functional/cms/forms_controller_test.rb +++ /dev/null @@ -1,120 +0,0 @@ -require 'test_helper' - -# Phase 4, stage F -- work item 4.4 / Tier B B9, criterion 7. -# Docs: docs/rails-upgrade/phase-4-implementation-plan.md -# -# Cms::FormsController was at 0% coverage: no functional test file existed, so nothing -# ever instantiated it. Two of its three before_action callbacks manipulate params as -# though they were a plain Hash, and both are on the create/update path: -# -# forms_controller.rb:26 params[:form][:field_ids] = params[:field_ids].split(" ") -# forms_controller.rb:33 params[:form].delete(:new_entry) -# -# :33 is one of the ten B9 sites. `.delete` still exists on -# ActionController::Parameters in Rails 5; what changed is that Parameters is no longer -# a Hash, so surrounding code assuming Hash semantics -- assignment into it, `.each` -# yielding pairs, implicit to_hash coercion, permitted-state propagation -- can behave -# differently. :26 is the assignment case and travels with it. -# -# `new_entry` is described in the source as "a garbage parameter that exists to make -# displaying forms work". It is not in Cms::Form's column list, so if the strip ever -# stopped working the failure would be an UnknownAttributeError on a real submission -- -# loud, but only in production, because nothing here exercised it until now. -module Cms - class FormsControllerTest < ActionController::TestCase - include Cms::ControllerTestHelper - - def setup - given_a_site_exists - login_as_cms_admin - end - - def create_form(form_attrs = {}, extra = {}) - post :create, params: { - field_ids: '', - form: {name: 'Contact Us'}.merge(form_attrs) - }.merge(extra) - end - - test "create strips the new_entry garbage parameter before assignment" do - assert_difference 'Cms::Form.count', 1 do - create_form(new_entry: 'garbage that is not a column') - end - - refute @controller.params[:form].key?(:new_entry), - "strip_new_entry_params must remove :new_entry from params[:form] -- it is " + - "not a column on cms_forms, so anything that mass-assigns it raises" - refute @controller.params[:form].key?('new_entry') - end - - test "create persists a form when new_entry is present" do - create_form(new_entry: 'garbage') - - form = Cms::Form.order(:id).last - assert_equal 'Contact Us', form.name, - "the form should save normally; :new_entry is discarded, not fatal" - end - - # The sibling callback, and the reason it is tested alongside the delete: it ASSIGNS - # into params[:form], which is the other Hash-shaped assumption on this path. - test "create splits the space separated field_ids into an array on the form params" do - # Built directly: there is no :form_field factory, which is itself a symptom of - # this subsystem never having been tested. - field_ids = 3.times.map do |n| - Cms::FormField.create!(label: "Field #{n}", field_type: 'text_field').id - end - - create_form({}, field_ids: field_ids.join(' ')) - - assert_equal field_ids.map(&:to_s), @controller.params[:form][:field_ids], - "associate_form_fields must write an Array back into params[:form]" - end - - test "new builds and saves a form with default confirmation text" do - assert_difference 'Cms::Form.count', 1 do - get :new - end - - assert_equal "Thanks for filling out this form.", - assigns(:block).confirmation_text, - "the controller's own work should happen even though the view " + - "cannot render -- see the characterization test below" - end - - # ------------------------------------------------------------------------- - # CHARACTERIZATION: the Forms admin UI cannot render. This is a live defect, - # found in Phase 4 stage F by instantiating a controller that had sat at 0% - # coverage, and it is NOT caused by the Rails upgrade -- it fails identically on - # both bundles. - # - # app/views/cms/forms/_form.html.erb:7 f.input :slug, as: :path - # app/inputs/path_input.rb:14 Cms::Section.with_path(object.class.path) - # - # but Cms::Form has no `path`, no `base_path` and no `slug` column, because - # `is_addressable path: '/forms'` is commented out at form.rb:5. So rendering the - # form partial raises NoMethodError and #new and #edit return 500. - # - # The same abandoned migration explains the :form factory, which set a `slug` that - # does not exist and had never been called by anything. - # - # NOT FIXED HERE. The repair is either restoring is_addressable (which needs a - # slug column, i.e. a migration) or removing the slug input -- a product decision - # about whether Forms are addressable, not an upgrade one. Out of scope for a - # characterization phase. - # - # This test pins the CURRENT behaviour. When someone fixes the UI it will fail -- - # that failure is the signal to delete this test, not to work around it. - # ------------------------------------------------------------------------- - test "CHARACTERIZATION: the new/edit views cannot render, on both bundles" do - refute Cms::Form.respond_to?(:path), - "Cms::Form gained a .path -- is_addressable was probably restored. If so " + - "the views below may now work; re-check and delete this test." - - get :new - assert_response :internal_server_error, - "expected the known PathInput failure. If this now succeeds, the " + - "Forms UI has been repaired -- delete this test and restore the " + - "assert_response :success above." - end - end -end From b78e347cbf28eee397cd22f03f8a0adb8074e6d8 Mon Sep 17 00:00:00 2001 From: Kris Hill Date: Fri, 18 Sep 2026 09:24:40 -0700 Subject: [PATCH 2/5] [CMS-434] more Forms-related removals --- app/assets/javascripts/cms/application.js | 1 - config/routes.rb | 18 ---- .../step_definitions/content_pages_steps.rb | 46 ----------- lib/cms/engine.rb | 1 - spec/inputs/name_input_spec.rb | 12 +-- test/dummy/config/application.rb | 1 - test/factories/factories.rb | 8 -- test/functional/cms/error_branches_test.rb | 82 ------------------- test/unit/belongs_to_optionality_test.rb | 17 ++-- 9 files changed, 15 insertions(+), 171 deletions(-) diff --git a/app/assets/javascripts/cms/application.js b/app/assets/javascripts/cms/application.js index 3a5d0ac35..ba1e93a48 100644 --- a/app/assets/javascripts/cms/application.js +++ b/app/assets/javascripts/cms/application.js @@ -11,7 +11,6 @@ //= require cms/core_library //= require cms/content_types //= require cms/attachment_manager -//= require cms/form_builder //= require cms/sitemap //= require bootstrap //= require bcms/ckeditor diff --git a/config/routes.rb b/config/routes.rb index 0f56979d0..5fe7e1cb7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -83,24 +83,6 @@ resources :attachments, :only => [:show, :create, :destroy] content_blocks :html_blocks - content_blocks :forms - resources :form_fields do - member do - get :confirm_delete - end - end - post "form_fields/:id/insert_at/:position" => 'form_fields#insert_at' - # get "/forms/:id/fields/preview" => 'form_fields#preview', as: 'preview_form_field' - - resources :form_entries do - collection do - post :submit - end - end - put "/form_entries" => "form_entries#bulk_update" - # Faux nested resource for forms (not sure if #content_blocks allows for it.) - # get 'forms/:id/entries' => 'form_entries#index', as: 'entries' - content_blocks :portlets post '/portlet/:id/:handler', :to => "portlet#execute_handler", :as => "portlet_handler" diff --git a/features/step_definitions/content_pages_steps.rb b/features/step_definitions/content_pages_steps.rb index 8e1277198..112182d5a 100644 --- a/features/step_definitions/content_pages_steps.rb +++ b/features/step_definitions/content_pages_steps.rb @@ -291,57 +291,11 @@ visit cms.new_tag_path end -Given /^I had created a form named "([^"]*)"$/ do |arg| - create(:form, name: arg) -end - -When /^I select forms from the content library$/ do - visit cms.forms_path -end - Given /^I am on the Groups page$/ do visit cms.groups_path should_see_a_page_titled "Groups" end -Then(/^I should see the list of forms$/) do - should_be_successful - should_see_a_page_titled('Forms') -end - -Then(/^I should see the "([^"]*)" form in the list$/) do |form_name| - page_should_have_content(form_name) -end - -When /^I am adding new form$/ do - visit cms.new_form_path -end - -When(/^I enter the required form fields$/) do - fill_in "Name", with: "Contact Us" - click_publish_button -end - -Then(/^after saving I should be redirect to the form page$/) do - should_be_successful - should_see_a_page_titled "Contact Us" -end - -When(/^I edit that form$/) do - @form = Cms::Form.where(name: "Contact Us").first - visit cms.edit_form_path(@form) -end - -When(/^I make changes to the form$/) do - fill_in "Name", with: "Updated Name" - click_publish_button -end - -Then(/^I should see the form with the updated fields$/) do - should_be_successful - should_see_a_page_titled "Updated Name" -end - Then /^I should be returned to the Assets page for "([^"]*)"$/ do |content_type| should_see_a_page_named("Assets") asset_selector_button.has_content?(content_type) diff --git a/lib/cms/engine.rb b/lib/cms/engine.rb index d102d0e30..cc9b3bb43 100644 --- a/lib/cms/engine.rb +++ b/lib/cms/engine.rb @@ -93,7 +93,6 @@ class Engine < Rails::Engine # Sets the default .css file that will be added to forms created via the Forms module. # Projects can override this as needed. - app.config.cms.form_builder_css = 'cms/default-forms' end diff --git a/spec/inputs/name_input_spec.rb b/spec/inputs/name_input_spec.rb index 64e45eb54..fff502b87 100644 --- a/spec/inputs/name_input_spec.rb +++ b/spec/inputs/name_input_spec.rb @@ -8,22 +8,22 @@ def name_input(attribute_name, object) NameInput.new(form_builder, attribute_name, attribute_name, :string) end + # These were skipped for years against Cms::Form, which stopped being addressable in + # b2e3df44 and has since been removed entirely. Dummy::Product is addressable and has + # a slug, which is what the input actually needs, so they run again. describe 'should_autogenerate_slug?' do it 'should generate slug when object is new' do - skip "Form addressibility removed 6 years ago. app/models/cms/form.rb:5" - input = name_input(:name, Cms::Form.new) + input = name_input(:name, Dummy::Product.new) input.send(:should_autogenerate_slug?).must_equal true end it 'should not generate slug for saved object with a name/slug' do - skip "Form addressibility removed 6 years ago. app/models/cms/form.rb:5" - input = name_input(:name, Cms::Form.create!(name: "Name", slug: "/name")) + input = name_input(:name, Dummy::Product.create!(name: "Name", slug: "/name")) input.send(:should_autogenerate_slug?).must_equal false end it 'should generate slug when object has blank name and slug' do - skip "Form addressibility removed 6 years ago. app/models/cms/form.rb:5" - input = name_input(:name, Cms::Form.create!(name: "", slug: "")) + input = name_input(:name, Dummy::Product.create!(name: "", slug: "")) input.send(:should_autogenerate_slug?).must_equal true end end diff --git a/test/dummy/config/application.rb b/test/dummy/config/application.rb index 387f3ff64..49f91b6b1 100644 --- a/test/dummy/config/application.rb +++ b/test/dummy/config/application.rb @@ -20,7 +20,6 @@ class Application < Rails::Application config.action_mailer.default_url_options = { :host => "localhost:3000" } - config.cms.form_builder_css = 'custom-forms' # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers # -- all .rb files in that directory are automatically loaded. diff --git a/test/factories/factories.rb b/test/factories/factories.rb index 91bf4f2e0..e65807cb9 100644 --- a/test/factories/factories.rb +++ b/test/factories/factories.rb @@ -279,12 +279,4 @@ product.sequence(:slug) { |n| "/product-#{n}" } end - # `slug` was removed here in Phase 4, stage F. Cms::Form has no slug attribute -- - # cms_forms has name, description, confirmation_* and notification_email -- so every - # call raised NoMethodError. Nothing noticed because nothing called it: this factory - # had zero usages anywhere in test/, spec/ or features/, which is the same 0%-coverage - # hole in the Forms subsystem that criterion 7 exists to close. - factory :form, :class => Cms::Form do |form| - form.sequence(:name) { |n| "Form #{n}" } - end end diff --git a/test/functional/cms/error_branches_test.rb b/test/functional/cms/error_branches_test.rb index ad6ce97ac..86f78b180 100644 --- a/test/functional/cms/error_branches_test.rb +++ b/test/functional/cms/error_branches_test.rb @@ -65,86 +65,4 @@ def setup end end - # form_fields_controller.rb:43. The failure half of `update`. Stage F covered - # `create` and `new` on this controller; `update` had no test at all, so neither - # half of its branch had ever run. - class FormFieldUpdateFailureBranchTest < ActionController::TestCase - tests Cms::FormFieldsController - include Cms::ControllerTestHelper - - def setup - given_a_site_exists - login_as_cms_admin - @form = create(:form, name: "Contact Us") - @taken = Cms::FormField.create!(label: "Email", field_type: "text_field") - @form.fields << @taken - @field = Cms::FormField.create!(label: "Phone", field_type: "text_field") - @form.fields << @field - @form.save! - end - - test "update renders the field as json on success" do - put :update, params: {id: @field.id, form_field: {label: "Mobile"}} - - assert_response :success - assert_equal "Mobile", JSON.parse(response.body)['label'] - end - - # ------------------------------------------------------------------------- - # CHARACTERIZATION: this branch cannot be reached through the controller. - # - # Three independent facts, each verifiable on its own, close every route to it: - # - # 1. `:name` is the ONLY validated attribute -- uniqueness scoped to :form_id - # (form_field.rb:18). Nothing else on the model can fail. - # 2. `:name` is assigned by `before_validation(on: :create)` (form_field.rb:14), - # so an update never recomputes it from the label. Posting a colliding label - # changes the label and leaves the name alone, and validation passes. - # 3. `:name` is explicitly removed from the permitted list -- - # `FormField.permitted_params` is `super - [:name]` (form_field.rb:67-69) -- - # so a request cannot set it directly either. - # - # So `field.update form_field_params` always succeeds, and - # form_fields_controller.rb:43 is dead code. - # - # Both of this test's first two drafts were wrong in instructive ways, which is why - # all three facts are asserted rather than described: a colliding **label** returned - # 200 (fact 2), and then a colliding **name** also returned 200 (fact 3). - # - # NOT FIXED -- and not obviously a defect. Re-deriving :name on update would break - # existing form entries, which the model says in its own comment at - # form_field.rb:10-12. The branch may simply be vestigial. - # ------------------------------------------------------------------------- - test "CHARACTERIZATION: update cannot fail, so the Fail branch is unreachable" do - assert_equal 1, Cms::FormField.validators.size, - "fact 1: :name uniqueness is the only validator" - refute_includes Cms::FormField.permitted_params, :name, - "fact 3: :name cannot be set through a request" - - # fact 2: a colliding label is accepted, because :name does not follow it - put :update, params: {id: @field.id, form_field: {label: @taken.label}} - - assert_response :success - assert_equal "Email", @field.reload.label, "the label did change" - assert_equal "phone", @field.name.to_s, "and the name did not, so nothing collided" - end - - # The branch is unreachable, but `render plain:` still has to be right, because - # `render text:` is removed at Rails 5.1 and this is one of the two sites Phase 3 - # converted. Reaching it means making `update` return false, which only a stub can - # do -- everything after that point is the real render. Same shape as stage G's - # version-conflict test, and for the same reason. - test "the Fail branch renders plain text when it is reached" do - Cms::FormField.any_instance.stubs(:update).returns(false) - - put :update, params: {id: @field.id, form_field: {label: "Anything"}} - - assert_response :internal_server_error - assert_equal "Fail", response.body - assert_equal "text/plain", response.content_type, - "`render text:` would answer text/html. This assertion is what " + - "proves the Phase 3 conversion took, and it is the only thing that " + - "will notice if it is reverted." - end - end end diff --git a/test/unit/belongs_to_optionality_test.rb b/test/unit/belongs_to_optionality_test.rb index 51685295b..4c5fe7e09 100644 --- a/test/unit/belongs_to_optionality_test.rb +++ b/test/unit/belongs_to_optionality_test.rb @@ -100,8 +100,6 @@ class BelongsToOptionalityTest < ActiveSupport::TestCase 'Cms::Tagging#tag' => :optional, 'Cms::Tagging#taggable' => :optional, # polymorphic, unvalidated 'Cms::SectionNode#node' => :optional, # polymorphic, unvalidated; a node is built before it is linked - 'Cms::FormEntry#form' => :optional, - 'Cms::FormField#form' => :optional, 'Cms::PageRouteOption#page_route' => :optional, 'Cms::GroupSection#group' => :optional, 'Cms::GroupSection#section' => :optional, @@ -126,7 +124,7 @@ class BelongsToOptionalityTest < ActiveSupport::TestCase # Classes carrying the literal declarations audited above. AUDITED_CLASSES = %w[ Cms::Category Cms::Connector Cms::PageRoute Cms::Task Cms::Group Cms::Attachment - Cms::Tagging Cms::SectionNode Cms::FormEntry Cms::FormField Cms::PageRouteOption + Cms::Tagging Cms::SectionNode Cms::PageRouteOption Cms::GroupSection Cms::UserGroupMembership Cms::GroupTypePermission Cms::GroupPermission ].freeze @@ -257,19 +255,22 @@ def verdict_for(klass, reflection) # The count, as a tripwire rather than as the enumeration itself. # # The sweeps above enumerate by reflection, which is deliberate -- it catches a - # belongs_to added tomorrow, which a hardcoded list of 29 could not. But Phase 4's - # criterion 4 asks that "the count in the test matches 29", and a reviewer checking + # belongs_to added tomorrow, which a hardcoded list of 27 could not. But Phase 4's + # criterion 4 asks that "the count in the test matches", and a reviewer checking # one number is a cheap and useful thing to preserve. This is that number. # + # It was 29 until the Forms subsystem was removed, which took Cms::FormEntry#form and + # Cms::FormField#form out of AUDIT along with their classes. + # # If it fails, a declaration was added or removed. Update AUDIT/BEHAVIOR_AUDIT with a # verdict for it first, then update this number -- not the other way round. - test "the audit still covers 29 belongs_to declarations" do + test "the audit still covers 27 belongs_to declarations" do literal = AUDIT.size # one entry per declaration on a named class injected = BEHAVIOR_AUDIT.size # userstamping x2, categorizing x1 dynamic = 2 # versioning.rb:115, dynamic_attributes.rb:171 - assert_equal 29, literal + injected + dynamic, - "expected 29 audited belongs_to declarations, got " + + assert_equal 27, literal + injected + dynamic, + "expected 27 audited belongs_to declarations, got " + "#{literal} literal + #{injected} behavior-injected + #{dynamic} dynamic" end From 86d5f21d350d33fdfc671979694a4bca0398c775 Mon Sep 17 00:00:00 2001 From: Kris Hill Date: Mon, 21 Sep 2026 10:19:15 -0700 Subject: [PATCH 3/5] [CMS-434] optionally quiet deprecation warnings and make reading test results easier for humans --- .simplecov | 6 +++++ script/test-summary | 59 ++++++++++++++++++++++++++++++++++++++++++ test/quiet_warnings.rb | 30 +++++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100755 script/test-summary create mode 100644 test/quiet_warnings.rb diff --git a/.simplecov b/.simplecov index 8def667a7..50d32d95e 100644 --- a/.simplecov +++ b/.simplecov @@ -1,3 +1,9 @@ +# Every test entry point (test/test_helper.rb, spec/minitest_helper.rb, +# features/support/env.rb) requires simplecov first, which loads this file, so this is +# the one place shared by all five suites. See test/quiet_warnings.rb for what it does +# and how to turn it off. +require_relative 'test/quiet_warnings' + # The suite runs as five separate processes (units, spec, functionals, orphans, # features) that merge through coverage/.resultset.json. SimpleCov discards any stored # result older than merge_timeout, which defaults to 600s -- so on a full run, diff --git a/script/test-summary b/script/test-summary new file mode 100755 index 000000000..2fe146fd8 --- /dev/null +++ b/script/test-summary @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# +# Run the test suite and print only the results. +# +# script/test-summary # the full suite (rake test) +# script/test-summary test:units # one group +# TEST_LOG=/tmp/x.log script/test-summary +# +# Full output, warnings and all, still goes to the log so a failure can be read in +# context. Two things this exists to get right: +# +# 1. The exit code is rake's. `rake test | tail` reports the exit code of `tail`, +# which is always 0 -- a piped run looks green over `rake aborted!`. +# 2. `rake test` runs five suites in separate processes and only the last one's +# summary lands at the end of the output, so a trailing "0 failures" says +# nothing about the other four. This prints every group's summary. +set -uo pipefail + +cd "$(dirname "$0")/.." + +LOG="${TEST_LOG:-tmp/test-run.log}" +mkdir -p "$(dirname "$LOG")" + +TASKS=("$@") +[ ${#TASKS[@]} -eq 0 ] && TASKS=("test") + +echo "running: rake ${TASKS[*]} (full output: $LOG)" +bundle exec rake "${TASKS[@]}" > "$LOG" 2>&1 +STATUS=$? + +rule() { printf '\n\033[2m%s\033[0m\n' "── $1 ──────────────────────────────────"; } + +rule "results" +grep -hE "^[0-9]+ (tests|runs|examples), |^[0-9]+ scenarios |^[0-9]+ steps " "$LOG" \ + || echo "(no suite summaries found -- the run probably died early; see $LOG)" + +# Failure and error detail, with enough trailing context to be actionable. +if grep -qE "^ *[0-9]+\) (Failure|Error)|^Failing Scenarios" "$LOG"; then + rule "failures" + grep -hE -A 6 "^ *[0-9]+\) (Failure|Error)" "$LOG" + grep -hE -A 20 "^Failing Scenarios" "$LOG" +fi + +# A group can fail while the tail of the log still reads "0 failures". +if grep -qE "^FAILED:|^rake aborted" "$LOG"; then + rule "aborted" + grep -hE "^FAILED:|^rake aborted|^Test failures in:" "$LOG" +fi + +rule "coverage" +grep -hE "^(Line|Branch) Coverage:" "$LOG" | tail -2 + +if [ $STATUS -eq 0 ]; then + printf '\n\033[32mPASS\033[0m (rake exit 0)\n' +else + printf '\n\033[31mFAIL\033[0m (rake exit %s) -- full output: %s\n' "$STATUS" "$LOG" +fi + +exit $STATUS diff --git a/test/quiet_warnings.rb b/test/quiet_warnings.rb new file mode 100644 index 000000000..aad3cb491 --- /dev/null +++ b/test/quiet_warnings.rb @@ -0,0 +1,30 @@ +# Minitest sets `Warning[:deprecated] = true` (minitest-5.19.0/lib/minitest.rb), which +# switches Ruby's deprecation category back on for every suite regardless of the +# `t.warning = false` on each Rake::TestTask. That is ~650 lines per full run, and all +# but a handful come from inside gems: Rails 4.2 tripping Ruby 2.7 deprecations in code +# we do not own and cannot fix before the Rails 5 hop. +# +# Filtering happens at warn-time rather than by resetting the flag, because minitest +# sets the flag when it loads -- which is after this file, whichever entry point is used. +# +# Only gem-origin warnings are dropped. Anything from browsercms, the dummy app or the +# stdlib still reaches you, which is the point: there is one in our own code right now, +# +# test/unit/models/sections_test.rb:179: warning: constant ::Fixnum is deprecated +# +# and Fixnum is gone in Ruby 3.2, so it must stay visible. +# +# Set VERBOSE_WARNINGS=1 to disable the filtering and see everything. +unless ENV['VERBOSE_WARNINGS'] + module QuietGemWarnings + def warn(message, *args, **kwargs) + return if message.to_s.include?('/gems/') + + # Ruby 2.7 warns when an empty kwargs splat is forwarded, which would make this + # filter a source of the noise it exists to remove. + kwargs.empty? ? super(message, *args) : super + end + end + + Warning.extend(QuietGemWarnings) +end From ca6906646e6d09d5f68ca7c4b8d054099d237ede Mon Sep 17 00:00:00 2001 From: Kris Hill Date: Mon, 21 Sep 2026 10:48:10 -0700 Subject: [PATCH 4/5] [CMS-434] update upgrade plan docs where relavent, improve guard for suppressing Ruby deprecation warnings --- .github/workflows/ci.yml | 17 +++++++++++++++-- RAILS_UPGRADE_TEST_PRIORITY.md | 20 +++++++++++--------- docs/rails-upgrade/phase-4-report.md | 12 ++++++------ docs/rails-upgrade/phase-5-the-5.0-bump.md | 6 ++++-- test/quiet_warnings.rb | 14 ++++++++++++-- 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90b626c5b..6e2d397ab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,7 +39,13 @@ jobs: PGHOST: localhost PGPORT: '5432' PGUSER: postgres - # Rails deprecations print regardless; this adds the Ruby-level ones. + # Rails deprecations print regardless (test.rb sets deprecation = :stderr); + # this adds the Ruby-level ones. + # + # It is also the off-switch for test/quiet_warnings.rb, which otherwise filters + # gem-origin Ruby warnings out of local runs -- ~650 lines a run, none of it from + # our own code. Setting this here keeps CI's record complete. Remove this and CI + # loses the Ruby-level warnings twice over. RUBYOPT: '-W:deprecated' # features/support/open_on_first_failure.rb pops the first failed page # open via Capybara's save_and_open_page -> Launchy -> xdg-open. Useful at @@ -181,8 +187,11 @@ jobs: # Phase 3 cleared all five and the job stayed red on ten others. Phase 4 # stage B cleared those, and the 5.0 bundle has been green since. # - # As of the end of Phase 4, both bundles run identically: + # As of the end of Phase 4, both bundles ran identically: # 838 unit / 145 spec / 139 functional / 7 orphan / 154 cucumber, 0F/0E. + # CMS-434 then removed the Forms subsystem and its tests. Do not treat the + # numbers above as the current expectation -- `script/test-summary` prints + # every group's summary in one place if you need today's. # # KEEP THIS JOB GATING. Three of the four root causes behind those ten # failures were NOT Rails 5 incompatibilities. They were live defects on @@ -199,6 +208,10 @@ jobs: # never render, and an attribute chain that answers nil where it should # raise. Every one fails identically on 4.2. # + # (The two Forms defects were closed by CMS-434, which removed the subsystem + # rather than repairing it -- unused in production, unreachable from the admin + # UI. The argument above is unaffected: they were still found this way.) + # # So this is not a Rails 5 canary. It is a second execution of the suite # under different framework semantics, and it has found more 4.2 bugs than # 5.0 ones. That is the argument for keeping it gating now that it passes. diff --git a/RAILS_UPGRADE_TEST_PRIORITY.md b/RAILS_UPGRADE_TEST_PRIORITY.md index 94cf86ac2..0989b4d45 100644 --- a/RAILS_UPGRADE_TEST_PRIORITY.md +++ b/RAILS_UPGRADE_TEST_PRIORITY.md @@ -44,7 +44,7 @@ Running the skill's `rails-42` and `rails-50` pattern sets against `app/` and `l | # | Finding | Location | Coverage | Why it matters | |---|---|---|---|---| | ➕A1 | **`belongs_to` blast radius is 29 declarations, not 24 — and 5 of them are injected into every model that uses a behavior** | the drafted 24 in `app/models/` **plus** `behaviors/userstamping.rb:16-17` (`created_by`, `updated_by`), `behaviors/categorizing.rb:16` (`category`), `behaviors/versioning.rb:115` (version row → parent), `behaviors/dynamic_attributes.rb:168` (`base_class`) | `userstamping.rb` 100%, `categorizing.rb` 88.89%, `versioning.rb` 96.91%, `dynamic_attributes.rb` 91.47% | The 24 static declarations affect 24 models. The 5 injected ones affect **every model in BrowserCMS and every downstream project that uses the behavior.** `created_by` / `updated_by` becoming required would fail every save made without a logged-in user — and the *only* existing `userstamping` tests are the nil-user cases, so they would go red immediately. That is fortunate, not sufficient: it is the one place the flag fails loudly. `Cms::Category#parent` (self-referential) and every polymorphic `belongs_to` fail silently into validation errors instead. | -| ➕A2 | **Two more `ActionController::Parameters`-as-Hash sites, both using methods the skill explicitly names** | `content_controller.rb:72` — `params.except(:controller, :action, :path)` inside `render_editing_frame`, passed to `ActionDispatch::Http::URL.url_for`; `path_helper.rb:33-36` — `params.clone` then `.delete` ×2 then `.merge!`, passed to `polymorphic_path` | `content_controller.rb:72` hit **153×**; `path_helper.rb:33-36` hit **49×** — both **covered** | B9 grows from 8 sites to 10. But note the asymmetry: these two are on well-exercised lines, so a `Parameters`-vs-`Hash` breakage here surfaces as a **loud CI failure**. The four sites in `form_fields_controller.rb:16`, `forms_controller.rb:33` (both **0% files**) and the untested branches of `pages_controller.rb:126-128` / `sections_controller.rb:43` are the dangerous ones. **This sharpens B9's priority rather than raising it: test the uncovered `.delete` sites, and let CI catch `.except` / `.clone` / `.merge!`.** | +| ➕A2 | **Two more `ActionController::Parameters`-as-Hash sites, both using methods the skill explicitly names** | `content_controller.rb:72` — `params.except(:controller, :action, :path)` inside `render_editing_frame`, passed to `ActionDispatch::Http::URL.url_for`; `path_helper.rb:33-36` — `params.clone` then `.delete` ×2 then `.merge!`, passed to `polymorphic_path` | `content_controller.rb:72` hit **153×**; `path_helper.rb:33-36` hit **49×** — both **covered** | B9 grows from 8 sites to 10. But note the asymmetry: these two are on well-exercised lines, so a `Parameters`-vs-`Hash` breakage here surfaces as a **loud CI failure**. The four sites in `form_fields_controller.rb:16`, `forms_controller.rb:33` (both **0% files**, and both **deleted in CMS-434**) and the untested branches of `pages_controller.rb:126-128` / `sections_controller.rb:43` are the dangerous ones — **two of them now**. **This sharpens B9's priority rather than raising it: test the uncovered `.delete` sites, and let CI catch `.except` / `.clone` / `.merge!`.** | | ➕A3 | **`HTML::FullSanitizer` — a Rails-4.2-era API kept alive by a transitive gem that cannot survive the 5.0 bump** | `lib/cms/content_filter.rb:12` — `HTML::FullSanitizer.new.sanitize(c[key]).strip`. Also asserted directly at `test/functional/cms/inline_controller_test.rb:7` | `content_filter.rb` **100%** (8 relevant lines) | The skill's `rails-42-patterns.yml` flags sanitizer usage as `kind: **breaking**`. `HTML::FullSanitizer` does not exist in Rails 4.2 itself — it comes from `rails-deprecated_sanitizer (1.0.4)`, pulled in only because `rails-dom-testing (1.0.9)` requires it. `rails-dom-testing 1.x` is capped at `activesupport < 5.0`, so **the moment Rails 5 resolves, `rails-dom-testing` goes to 2.x, `rails-deprecated_sanitizer` disappears from the bundle, and `content_filter.rb:12` raises `NameError`.** Loud and covered, so Tier C — but it is a *gem-topology* failure invisible to a code grep, which is exactly what the skill's Step 4.6 boot smoke test exists to catch. Rewrite to `ActionView::Base.full_sanitizer` / `Rails::Html::FullSanitizer` before the bump. Note `inline_controller_test.rb:7` asserts on the doomed gem — the draft correctly observed this test says nothing about its controller; it turns out to be worse than useless, it is a test of a dependency about to be removed. | | ➕A4 | **`.deliver!` at a second, *uncovered* site** | `email_message.rb:18` — `m.deliver!`. (`:15` is `def self.deliver!`, a definition, not a call.) The draft found only `:58` `.deliver` | `:58` hit **14×** (covered); **`:18` hit 0× — uncovered** | The draft listed 1 site and it was the covered one. The skill's `rails-42` pattern covers `.deliver!` as well as `.deliver`, and the bang form needs `deliver_now!`, not `deliver_now`. An uncovered mail-delivery call on the form-notification path is exactly the class of thing that fails in production rather than CI. | | ➕A5 | **`File.exists?` at 5 sites, not 1 — two of them uncovered** | `list_portlet.rb:22` (**0 hits**), `lib/cms/caching.rb:42` (24 hits), `lib/cms/attachments/attachment_serving.rb:44` (9 hits), `lib/tasks/core_tasks.rake:51`, `content_block_generator.rb:26` (the only one the draft found) | `list_portlet.rb` 36.36% / 14 missed; `caching.rb` 100%; `attachment_serving.rb` 88.46% | Undercounted 5× in the original draft. `caching.rb` and `attachment_serving.rb` are live request-path code, not generator code. Mechanical fix (`File.exist?`), no test needed — but the count matters for scoping. | @@ -273,8 +273,8 @@ Two upgrade interactions: `default_scope` composition semantics have shifted (no ``` app/controllers/cms/pages_controller.rb:126-128 params[:page].delete :hidden / :archived / :visibility app/controllers/cms/sections_controller.rb:43 params[:section].delete('group_ids') -app/controllers/cms/form_fields_controller.rb:16 params[:form_field].delete(:form_id) -app/controllers/cms/forms_controller.rb:33 params[:form].delete(:new_entry) +app/controllers/cms/form_fields_controller.rb:16 params[:form_field].delete(:form_id) ← REMOVED (CMS-434) +app/controllers/cms/forms_controller.rb:33 params[:form].delete(:new_entry) ← REMOVED (CMS-434) app/helpers/cms/path_helper.rb:34-35 filtered_params.delete(:action) / (:controller) app/helpers/cms/path_helper.rb:33,36 params.clone → .merge!(:order => ...) → polymorphic_path ← NEW app/controllers/cms/content_controller.rb:72 params.except(:controller, :action, :path) → url_for ← NEW @@ -295,12 +295,14 @@ Coverage status makes this worse than it looks: - `pages_controller.rb#strip_visibility_params` — **3 of its lines are untested** (85.87% file) - `sections_controller.rb:43` — inside the 9 missed lines -- `form_fields_controller.rb:16` — **0% file** -- `forms_controller.rb:33` — **0% file** +- ~~`form_fields_controller.rb:16` — **0% file**~~ — **file deleted in CMS-434** +- ~~`forms_controller.rb:33` — **0% file**~~ — **file deleted in CMS-434** - `path_helper.rb:33-36` — inside `sortable_column_path` (`:32`); **these lines are covered (49 hits)**; the file's 14 missed lines are concentrated in `link_to_usages` (`:40`) and `engine` (`:65`) - `content_controller.rb:72` — **covered (153 hits)**; file is 92.16% -So four of the ten sites are in code with **no test at all**, and those four are the entire job here. These are cheap tests (assert the stripped key is absent from the resulting record) and they sit directly on a security boundary: `strip_visibility_params` and the `group_ids` deletion are *authorization* logic — they exist to stop a non-admin from setting fields they shouldn't. `form_fields_controller.rb` and `forms_controller.rb` are both **0%-coverage files**, which is why the Forms subsystem holds its rank in §6. +So four of the ten sites were in code with **no test at all**, and those four were the entire job here. ⚠️ **Two of the four are gone as of CMS-434**, which deleted the Forms subsystem: `form_fields_controller.rb:16` and `forms_controller.rb:33`. **The job is now two sites**, both of them the security-relevant ones — `strip_visibility_params` and the `group_ids` deletion are *authorization* logic, existing to stop a non-admin setting fields they shouldn't. These are cheap tests: assert the stripped key is absent from the resulting record. + +The removal also retires the reason the Forms subsystem held its rank in §6 — it was ranked on two 0%-coverage controllers that no longer exist. ### B10. Zeitwerk and `require_dependency` @@ -329,9 +331,9 @@ Ordered by how much code sits behind them. **None of these need a test to be *fo | `respond_with` + class-level `respond_to` | **already breaking at 4.2** ✏️ | 6 | ✏️ **Promoted out of Tier C into fix-before-bump.** `rails-42-patterns.yml` classifies both as `kind: **breaking**` on the *current* version, not a future one. Works today only because `devise` pulls `responders 2.4.1` in transitively — `Gemfile.lock:142` confirms it arrives as a Devise dependency and **nothing in `browsercms.gemspec` declares it.** Declare it explicitly now: one line, zero risk, and it defuses the landmine *before* the Devise upgrade can move it. `content_controller.rb:79` is the main page-serving path (202 hits — CI will catch a regression). | | `HTML::FullSanitizer` | **breaks at 5.0** ➕ | 1 + 1 test | ➕ **New (A3) — missed entirely by the original sweep.** `content_filter.rb:12`. `rails-42-patterns.yml` flags sanitizer usage as `kind: breaking`. The constant comes from `rails-deprecated_sanitizer (1.0.4)`, in the bundle only because `rails-dom-testing (1.0.9)` requires it — and that gem is capped at `activesupport < 5.0`. **Resolving Rails 5 removes the gem and `content_filter.rb:12` raises `NameError`.** File is 100% covered so it fails loudly, but a code grep cannot see it — this is the failure class the skill's Step 4.6 boot smoke test exists for. Also: `test/functional/cms/inline_controller_test.rb:7` asserts on the doomed gem directly. | | `before_filter` / `after_filter` / `around_filter` / `skip_before_filter` | 5.1 ✅ | **37 across 17 files** | ✅ `rails-51-patterns.yml` → `FILTER_METHODS`, `kind: breaking`. Mechanical rename to `_action`. Boot-time failure. Safe to do now — `_action` works on 4.2. | -| `render :text =>` / `render text:` | 5.1 ✅ | 4 | ✅ `upgrade-5.0-to-5.1.md` §2 + `rails-51-patterns.yml` → `RENDER_TEXT`. → `render plain:` (skill: `render html:` if HTML was intended). **Both production sites are on untested error branches:** `content_block_controller.rb:138` ("Not Implemented") and `form_fields_controller.rb:43` ("Fail", 500). Worth a test *because* they're error paths nothing exercises. | +| `render :text =>` / `render text:` | 5.1 ✅ | 4 | ✅ `upgrade-5.0-to-5.1.md` §2 + `rails-51-patterns.yml` → `RENDER_TEXT`. → `render plain:` (skill: `render html:` if HTML was intended). **Both production sites are on untested error branches:** `content_block_controller.rb:138` ("Not Implemented") and `form_fields_controller.rb:43` ("Fail", 500). Worth a test *because* they're error paths nothing exercises. ⚠️ **CMS-434 deleted `form_fields_controller.rb`**, so this is now **one** production site; its Phase 4 test went with the file. | | `Relation#uniq` | 5.1 ✅ | 1 | ✅ `rails-51-patterns.yml` → `RELATION_UNIQ`. `section_nodes_controller.rb:75` → `.distinct`. Inside `nodes_to_update_on_success` (`:73`), which has **2 untested lines**, in a file at 41.18% whose `move_to_position` has 9 more. | -| `.deliver` / `.deliver!` | 5.0 ✅ (deprecated **at 4.2**) | **2** ➕ | ✅ `rails-42-patterns.yml`, `kind: deprecation` — **already warning on the current version, so fixable today.** ➕ **Undercounted (A4):** `email_message.rb:58` `.deliver` (14 hits, covered) **and `email_message.rb:18` `m.deliver!` — 0 hits, uncovered.** The bang form needs `deliver_now!`, not `deliver_now`. (`:15` `def self.deliver!` is a definition, not a call.) On the form-notification path (`form_entries_controller#submit`, **0% covered**). | +| `.deliver` / `.deliver!` | 5.0 ✅ (deprecated **at 4.2**) | **2** ➕ | ✅ `rails-42-patterns.yml`, `kind: deprecation` — **already warning on the current version, so fixable today.** ➕ **Undercounted (A4):** `email_message.rb:58` `.deliver` (14 hits, covered) **and `email_message.rb:18` `m.deliver!` — 0 hits, uncovered.** The bang form needs `deliver_now!`, not `deliver_now`. (`:15` `def self.deliver!` is a definition, not a call.) On the form-notification path (`form_entries_controller#submit`, **0% covered**). ⚠️ **CMS-434 deleted that caller.** `email_message.rb:18` `m.deliver!` still exists and is still uncovered — the conversion is still required — but the path that reached it is gone, so it is no longer reachable from form submission. | | `config.serve_static_assets` | 5.0 ➕ | 2 | ➕ **New (A6).** `test/dummy/config/environments/test.rb:11` and `production.rb:20` → `config.public_file_server.enabled`. `rails-42-patterns.yml`, `kind: deprecation`. **These are in the dummy app the entire suite boots against — this is a Phase 0 harness item, not application backlog.** Easy to miss because it isn't application code. | | `update_attributes` / `update_attributes!` | **6.0** ✏️ (was drafted 7.0) | **16** | ✏️ **Corrected — a full hop earlier (C7).** `upgrade-5.2-to-6.0.md` §3 and `references/breaking-changes-by-version.md` both place this at 5.2 → 6.0. → `update` / `update!`, which work identically on 4.2, so **make this change now.** Note `guest_user.rb:48` *defines* `update_attributes` — check callers before renaming. | | `require_dependency` (Zeitwerk) | **6.0** ✏️ (was drafted 7.0) | 1 | ✏️ **Corrected (C8).** `content_types_controller.rb:1`; file is **0% covered**. See B10 — this is the tip of the largest item in the plan, and it arrives two hops sooner than the draft assumed. | @@ -434,7 +436,7 @@ The question this document exists to answer. Measured against the skill's `refer |---|---|---| | Unit / model tests | 91.82%, 47 files | ✅ **Adequate.** Also the layer Rails 5→8 changes least. | | Library / concern tests | 73.04%, 91 files | ⚠️ **Adequate in aggregate, wrong in distribution.** The monkeypatches and behaviors carrying the upgrade risk are the thin ones (`dynamic_attributes` 0.19x test-to-source, `schema_dumper` 62.5%). Tier B targets exactly these. | -| Controller tests | **57.71%**, 612 missed lines | ❌ **Not adequate.** This is where params, `render :text`, and `*_filter` changes land. Four 0% files (`form_entries` 140 lines, `form_fields` 74, `forms` 35, `content_types` 18) hold two Tier-C breakages and three of B9's four dangerous sites. | +| Controller tests | **57.71%**, 612 missed lines | ❌ **Not adequate.** This is where params, `render :text`, and `*_filter` changes land. Four 0% files (`form_entries` 140 lines, `form_fields` 74, `forms` 35, `content_types` 18) hold two Tier-C breakages and three of B9's four dangerous sites. ⚠️ **Stale as of CMS-434:** three of those four files were deleted with the Forms subsystem, leaving `content_types` (18 lines) and two of B9's dangerous sites. The percentage predates the removal. | | Integration / multi-step workflows | none in this repo | ❌ **Absent.** No test chains create → edit → publish → connect → render → version → revert. See `TEST_COVERAGE_ANALYSIS.md` §Phase 3. | | System / feature (JS, forms, uploads, navigation) | 53 Cucumber features, 4,860 LOC | ⚠️ **Exists but brittle and unmeasured.** On Poltergeist/PhantomJS (abandoned 2018), `aruba` hard-pinned, `@cli` features excluded from the default task. **The true pass rate is still unknown** — establish it before relying on it. | | Auth / authorization | permission join-models untested; `persistent_user.rb` (209 LOC) has no test | ❌ **Not adequate**, and the worst possible regression (permissions failing *open*). B9's `strip_visibility_params` and `group_ids` deletion are authorization logic on untested lines. | diff --git a/docs/rails-upgrade/phase-4-report.md b/docs/rails-upgrade/phase-4-report.md index 0555f7669..92f20829e 100644 --- a/docs/rails-upgrade/phase-4-report.md +++ b/docs/rails-upgrade/phase-4-report.md @@ -38,15 +38,15 @@ Phase 4 was scoped to write characterization tests: pin what the code does *toda | Publishing a non-versioned record silently did nothing for years | B | **yes** | | `?some_id=` blank in a URL was a 500 | B | **yes** | | The edit-conflict screen raised `MissingTemplate` — two broken partial paths, not one | G | **yes** | -| Public form submission 500s for every form showing confirmation text | F | **yes** | -| The Forms admin UI 500s | F | **yes** | +| Public form submission 500s for every form showing confirmation text ✅ *resolved by CMS-434 — Forms removed* | F | **yes** | +| The Forms admin UI 500s ✅ *resolved by CMS-434 — Forms removed* | F | **yes** | | `Cms::ToolbarController` is routed but can never render | F | **yes** | | Every version saved through the CMS is commented with the whole record | G | **yes** | | `Model.exists?` with no arguments raises on every soft-deleting model | H | **yes** | | `read_attribute` returns **nil** on every portlet instead of raising | H | **yes** | | `nonversioned_class` raises `FrozenError` in the only case it exists for | H | **yes** | | `move_to_position`'s rescue cannot report either lookup failure | I | **yes** | -| `form_fields_controller#update` cannot fail at all | I | **yes** | +| `form_fields_controller#update` cannot fail at all ✅ *moot as of CMS-434 — controller removed* | I | **yes** | | `Cms::Section#pages` returned rows in arbitrary order | I | **yes** | **Fourteen of the fifteen fail identically on Rails 4.2.** None was caused by the upgrade. The upgrade was the excuse to look. @@ -77,7 +77,7 @@ That is the finding worth carrying out of this phase: **the dual-boot suite is n | `read_attribute` private and answering nil on portlets | Changes the public surface of every portlet class in every installation | | `respond_to?` disagrees with `method_missing` | A correct `respond_to_missing?` would have to return true for every name | | `nonversioned_class` raises `FrozenError` | Unreachable in this engine; the repair enables a path that has never run anywhere | -| Public form submission and the Forms admin UI 500 | Both need a product decision about the Forms subsystem's abandoned addressable migration | +| Public form submission and the Forms admin UI 500 | Both need a product decision about the Forms subsystem's abandoned addressable migration. ✅ **Answered in CMS-434: remove.** Production held zero forms, zero entries and zero connectors, and the subsystem was unreachable from the admin UI | | `Cms::ToolbarController` is vestigial | Deleting a routed controller is the admin UI owner's call | | `move_to_position`'s rescue raises on lookup failure | The repair means deciding what the error says without the objects that failed to load | @@ -153,14 +153,14 @@ Hence the rule adopted mid-phase and applied for the rest of it: **verify that a - [ ] **B4 — the three untested Paperclip validation macros.** Deliberately out of scope: the phase document scopes it to validation tests with no replacement. ⚠️ It needs a destination in the Phase 5 or Phase 6 document rather than lapsing here. `validates_attachment_presence` is also **defined twice** (`attaching.rb:89` and `:98`), the first silently overwritten. - [ ] **The `or` half of B8.** `ActiveRecord::Relation#or` arrives in Rails 5.0, so a test using it cannot pass on the `Gemfile` bundle and criterion 11 forbids version branching. Measured on 5.0 — the default scope distributes correctly across both sides — and handed to Phase 5 with the answer attached. -- [ ] **The ten characterized defects in [§3](#3-what-was-fixed-and-what-was-deliberately-left).** Each needs a ticket and a product decision. The two worst are public form submission 500ing for unauthenticated visitors, and optimistic locking being silently defeated. +- [ ] **The ten characterized defects in [§3](#3-what-was-fixed-and-what-was-deliberately-left).** Each needs a ticket and a product decision. The two worst are public form submission 500ing for unauthenticated visitors, and optimistic locking being silently defeated. ✅ **Three are now closed by CMS-434** (both Forms 500s and `form_fields_controller#update`), leaving **seven**; of the two named worst, only optimistic locking survives. Their pinning tests went with the code. ## 9. For Phase 5 Phase 5 is the bump itself. Three things from here bear on it: 1. **The `next-rails` job should stay gating.** It is green now, and the argument for keeping it is in [`ci.yml`](../../.github/workflows/ci.yml): it has found more 4.2 bugs than 5.0 ones. -2. **`form_entries_controller` is on Phase 5's manual-verification list** and now has 10 tests, which replaces part of that manual pass with something that runs every build. It is also where the most serious open defect lives. +2. ~~**`form_entries_controller` is on Phase 5's manual-verification list**~~ ✅ **Superseded by CMS-434.** The controller, its 10 tests and the whole Forms subsystem were removed, and the Forms bullet has been struck from Phase 5 §5.5. What was the most serious open defect here — public submission 500ing for unauthenticated visitors — is closed by deletion rather than repair. 3. **Two 5.1 landmines are now covered rather than merely converted:** `render text:` at two sites and the two-argument `connection.quote` in `publishing.rb`. The tests assert behaviour rather than API shape, so they survive the removal and fail only if the fix for it is wrong. The Zeitwerk inventory from stage C also scopes **Phase 6**: one misnamed file, and an `autoload_paths` block in `engine.rb` that is at most one entry additive and can mostly be deleted. diff --git a/docs/rails-upgrade/phase-5-the-5.0-bump.md b/docs/rails-upgrade/phase-5-the-5.0-bump.md index 6b6f5464b..81ad980e3 100644 --- a/docs/rails-upgrade/phase-5-the-5.0-bump.md +++ b/docs/rails-upgrade/phase-5-the-5.0-bump.md @@ -95,7 +95,7 @@ The [§7 coverage-adequacy table](../../RAILS_UPGRADE_TEST_PRIORITY.md) has four - [ ] **Auth / authorization** — log in, log out, password reset, role-based access. The permission join-models are untested and `persistent_user.rb` (209 LOC) has no test at all; **permissions failing *open* is the worst possible upgrade regression.** - [ ] **Content-block CRUD end to end** — create, edit, publish, connect to a page, render on the public page, view version history, revert. - [ ] **File upload / download / image variants** — Paperclip is still in place, but this is the public upload path. -- [ ] **Forms** — submission, validation display, CSRF. `form_entries_controller.rb` was 140 lines at **0% coverage**; [Phase 4](phase-4-report.md) gave it 10 tests, which replaces part of this manual pass with something that runs every build. ⚠️ It also found that **public form submission 500s** for every form configured to show confirmation text — `Cms::Form.layout` does not exist. That is a **known pre-existing defect**, characterized not fixed: do not read it as a bump regression. +- [x] ~~**Forms** — submission, validation display, CSRF.~~ **Nothing to verify: the Forms subsystem was removed in CMS-434.** Phase 4 characterized two live 500s here (public submission, and the admin UI) and left the product decision open. The decision came back "remove": production had zero forms, zero entries and zero page connectors, the type was unreachable from the admin UI in `cms`, and the site's real forms are third-party Cognito embeds in HTML blocks. The models, controllers, views, routes — including the unauthenticated `submit` endpoint — and their tests are gone. **This removes a manual-verification item rather than deferring one.** - [ ] **Assets** — CSS, JS, images load; fingerprinting and compilation work. - [ ] Record the results. An unrecorded manual pass is indistinguishable from no manual pass. @@ -111,7 +111,9 @@ Three items Phase 4 could not close in its own scope. **They are listed here so ``` Once this phase lands, the test is one line. Add it to [`soft_deleting_test.rb`](../../test/unit/behaviors/soft_deleting_test.rb). - [ ] **Resolve the units suite's mixed database-cleaning strategies, before the bump.** `ActiveSupport::TestCase` rolls each test back in a transaction while [`publishing_mini_test.rb`](../../test/unit/behaviors/publishing_mini_test.rb) — a `Minitest::Spec` inside the units glob — truncates the whole database after every example. Phase 4 saw **two intermittent failures across ~14 full runs** and isolated only one of them ([report §6](phase-4-report.md#6-an-open-question-suite-stability)). One spec file is the entire exposure. ⚠️ Worth doing **first**: a suite that fails one run in seven is at its most expensive during a bump, when every red build is already suspect. -- [ ] **File tickets for the ten characterized defects.** Phase 4 left ten live defects deliberately unfixed, each pinned by a test that goes red when it is repaired — they are tabulated in [§3 of the report](phase-4-report.md). ⚠️ **These are not Phase 5 work**, and fixing them during the bump would confuse an upgrade regression with a pre-existing one. They need tickets and product decisions. The two worst: **public form submission 500s for unauthenticated visitors**, and **optimistic locking is silently defeated** on every versioned content type. +- [ ] **File tickets for the seven remaining characterized defects.** Phase 4 left ten live defects deliberately unfixed, each pinned by a test that goes red when it is repaired — they are tabulated in [§3 of the report](phase-4-report.md). ⚠️ **These are not Phase 5 work**, and fixing them during the bump would confuse an upgrade regression with a pre-existing one. They need tickets and product decisions. The worst of them is **optimistic locking is silently defeated** on every versioned content type. + + **Three of the original ten are closed by CMS-434**, which removed the Forms subsystem rather than repairing it: public form submission 500s, the Forms admin UI 500s, and `form_fields_controller#update` cannot fail at all. ⚠️ Their pinning tests were deleted with the code, so the "goes red when repaired" mechanism no longer covers those three — there is nothing left to repair. Do not go looking for them in the suite. ### 5.6 — Ship it diff --git a/test/quiet_warnings.rb b/test/quiet_warnings.rb index aad3cb491..e875495b5 100644 --- a/test/quiet_warnings.rb +++ b/test/quiet_warnings.rb @@ -14,8 +14,18 @@ # # and Fixnum is gone in Ruby 3.2, so it must stay visible. # -# Set VERBOSE_WARNINGS=1 to disable the filtering and see everything. -unless ENV['VERBOSE_WARNINGS'] +# Two ways to switch the filtering off and see everything: +# +# VERBOSE_WARNINGS=1 -- the short local form +# RUBYOPT='-W:deprecated' -- asking Ruby for the warnings directly +# +# The second is what CI already does. .github/workflows/ci.yml sets +# `RUBYOPT: '-W:deprecated'` on the 4.2 `test` job specifically to surface Ruby-level +# deprecations, so keying off it means this filter cannot quietly undo that: if you have +# asked Ruby for the warnings, you get all of them. The `next-rails` job does not set it, +# which is unchanged from before this file existed -- Ruby-level warnings were never +# enabled there. +unless ENV['VERBOSE_WARNINGS'] || ENV['RUBYOPT'].to_s.include?('-W:deprecated') module QuietGemWarnings def warn(message, *args, **kwargs) return if message.to_s.include?('/gems/') From b8bc5a071c41b57dad6cafa90158715b099b60e8 Mon Sep 17 00:00:00 2001 From: Kris Hill Date: Thu, 24 Sep 2026 15:52:07 -0700 Subject: [PATCH 5/5] [CMS-434] update actual measured coverage in the code that gates tests passing on coverage numbers, add test to cover scenario lost with forms removal --- lib/tasks/core_tasks.rake | 64 ++++++++++++-- test/unit/lib/acts_as_list_test.rb | 132 +++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 test/unit/lib/acts_as_list_test.rb diff --git a/lib/tasks/core_tasks.rake b/lib/tasks/core_tasks.rake index 494109f36..f27567633 100644 --- a/lib/tasks/core_tasks.rake +++ b/lib/tasks/core_tasks.rake @@ -33,15 +33,33 @@ namespace :coverage do # meeting a threshold set for the whole chain. coverage/.last_run.json is # written unconditionally, and the last suite to finish writes the fully # merged figure, so one check after the chain is both correct and enough. - desc 'Fail if merged coverage fell below the recorded Phase 0 baseline' + desc 'Fail if merged coverage fell below the recorded baseline' task :check do require 'json' - # 78.35, not the 75.82 Phase 0 recorded: the simplecov 0.12 -> 0.22 bump - # changed the instrument, not the tests. Measured across that bump on - # identical code, the covered-line count was identical at 4901 and only the - # denominator moved, 6460 -> 6255, because 0.18+ narrowed what counts as a - # relevant line. See docs/rails-upgrade/phase-2-harness-report.md. - threshold = Float(ENV.fetch('COVERAGE_MINIMUM', '78.35')) + # Phase 0 recorded 75.82 and this was 78.35 for everything up to CMS-434: not a + # raise, a re-measurement. The simplecov 0.12 -> 0.22 bump changed the instrument, + # not the tests -- across that bump on identical code the covered-line count was + # identical at 4901 and only the denominator moved, 6460 -> 6255, because 0.18+ + # narrowed what counts as a relevant line. See + # docs/rails-upgrade/phase-2-harness-report.md. + # + # RAISED 78.35 -> 83.71 AFTER CMS-434. This is the first time the line floor has + # moved since Phase 0, and it is catching up rather than recording new work: five + # phases of tests had lifted the measured figure 5.5 points above the floor, which + # is exactly the "headroom silently absorbs the first regression" the branch floor's + # note below argues against. Deleting every Forms test would not have tripped it. + # + # 83.71 IS THE 5.0 FIGURE, AND THAT IS DELIBERATE -- do not raise it to 4.2's. The + # two bundles do not agree on lines, measured on cleared resultsets at this commit: + # + # 4.2.11.3 4942 / 5895 83.83 + # 5.0.7.2 4935 / 5895 83.71 + # + # Same denominator, seven fewer covered lines on 5.0. One floor gates both jobs, so + # it tracks the lower bundle or the next-rails job goes red on a difference that is + # about framework internals, not about this repo's tests. If the gap ever closes, + # re-measure both and raise to the new lower figure. + threshold = Float(ENV.fetch('COVERAGE_MINIMUM', '83.71')) path = 'coverage/.last_run.json' abort "#{path} is missing -- did the suite run?" unless File.exist?(path) @@ -98,10 +116,40 @@ namespace :coverage do # and the first coverage section_nodes_controller has ever had. Each raise was # measured on a cleared resultset before being written here. # + # --------------------------------------------------------------------------- + # LOWERED 70.97 -> 70.63 BY CMS-434, which removed the Forms subsystem. Read this + # before "restoring" 70.97. + # + # Nothing that survives became less tested. The removed code was better tested than + # the average of what remains, so taking it out pulled the mean down. Measured on + # cleared resultsets, 4.2 bundle, either side of the removal: + # + # before (9a6427ee) 1038 / 1461 71.05% + # after 1001 / 1420 70.49% + # + # 41 branches went with the Forms files and 35 of them were covered -- 85%, against + # a suite average of 71%: + # + # form_field.rb 14/15 form_entries_controller.rb 8/12 + # form_entry.rb 5/6 form_fields_controller.rb 4/4 + # form.rb 4/4 + # + # The other two of the 37 lost are in lib/acts_as_list.rb, and they were the one + # real loss: Cms::FormField was the only caller passing a Symbol :scope, and the + # two that remain -- Cms::Connector (String scope) and Cms::SectionNode (default) + # -- both take the else branch, so `acts_as_list :scope => :some_association` + # became live engine code with no test. Untested, not unreachable, so it got one: + # test/unit/lib/acts_as_list_test.rb, the first test the file has ever had. + # + # THAT PUTS THE FLOOR AT 70.63, NOT 70.49. Reclaiming those two is the whole of + # the difference -- 1003/1420 against 1001/1420 -- and both bundles measure the + # same, so one floor still serves 4.2 and 5.0. + # --------------------------------------------------------------------------- + # # Clear coverage/.resultset.json before trusting either number. The five suites merge # through it with a 3600s timeout and both bundles use the same suite names, so a # partial or cross-bundle run leaves entries that shift the merged percentage. - branch_threshold = Float(ENV.fetch('COVERAGE_MINIMUM_BRANCH', '70.97')) + branch_threshold = Float(ENV.fetch('COVERAGE_MINIMUM_BRANCH', '70.63')) branch = result['branch'] # Print both figures before aborting, so a run that fails one gate still tells you diff --git a/test/unit/lib/acts_as_list_test.rb b/test/unit/lib/acts_as_list_test.rb new file mode 100644 index 000000000..8dca78f36 --- /dev/null +++ b/test/unit/lib/acts_as_list_test.rb @@ -0,0 +1,132 @@ +require 'test_helper' + +# CMS-434 fallout -- the Symbol form of `acts_as_list :scope`. +# +# WHY THIS FILE EXISTS +# +# Nothing in the suite has ever targeted lib/acts_as_list.rb. Every branch it had was +# reached incidentally, by the three models that declare it doing other things, and +# that was enough to cover the macro's two spellings of :scope: +# +# Cms::Connector (connector.rb:10) String scope -- the else branches +# Cms::SectionNode (section_node.rb:26) default scope "1 = 1" -- ditto +# Cms::FormField Symbol scope -- the then branches +# +# Removing the Forms subsystem took Cms::FormField with it, and with it the only +# caller in the engine that passes a Symbol. Measured: acts_as_list.rb went 29/46 +# branches to 27/46, the two lost being acts_as_list.rb:35 (append `_id` to a bare +# symbol) and acts_as_list.rb:38 (generate the Symbol form of `scope_condition`). +# +# The code is still shipped and still documented in the macro's own comment +# (acts_as_list.rb:26-30), and downstream applications built on this engine use it. +# Untested, not unreachable -- so it gets a test rather than a deletion. +# +# WHAT IS ACTUALLY AT RISK HERE +# +# `scope_condition` is built by string interpolation and spliced raw into a WHERE +# clause (`higher_item`, `lower_item`, `bottom_item`). A change to the generated text +# is a change to SQL, and the only thing that catches a malformed one is a query that +# runs. So the behavioural tests below create real rows rather than asserting on the +# generated string alone. + +# Top level, not under `Cms`, deliberately: namespaces_test.rb enumerates the +# constants in that namespace at load time and would adopt a test-only model as +# engine surface. Same reasoning as publishing_sql_test.rb. +ActiveRecord::Base.connection.instance_eval do + drop_table(:scoped_list_items) if table_exists?(:scoped_list_items) + create_table(:scoped_list_items) do |t| + t.integer :todo_list_id + t.integer :position + end +end + +# The documented spelling: a bare association name, which the macro turns into +# `todo_list_id` at acts_as_list.rb:35. +class ScopedListItem < ActiveRecord::Base + self.table_name = 'scoped_list_items' + acts_as_list :scope => :todo_list +end + +# The same scope written out. This one skips acts_as_list.rb:35 (the name already ends +# in `_id`) and must arrive at an identical condition -- the regression it guards +# against is a fix to the `_id` logic that produces `todo_list_id_id`. +class PreSuffixedScopedListItem < ActiveRecord::Base + self.table_name = 'scoped_list_items' + acts_as_list :scope => :todo_list_id +end + +class ActsAsListTest < ActiveSupport::TestCase + + def teardown + ScopedListItem.delete_all + end + + # --------------------------------------------------------------------------- + # The generated condition (acts_as_list.rb:38-45) + # --------------------------------------------------------------------------- + + test "a Symbol scope generates a condition naming the foreign key" do + item = ScopedListItem.new(:todo_list_id => 7) + assert_equal "todo_list_id = 7", item.scope_condition + end + + test "a Symbol scope generates IS NULL rather than `= ` when the key is nil" do + item = ScopedListItem.new(:todo_list_id => nil) + # The nil arm exists because the interpolated form would emit `todo_list_id = ` + # and take down every query the condition is spliced into. + assert_equal "todo_list_id IS NULL", item.scope_condition + end + + test "`_id` is appended once, whether or not the caller wrote it" do + assert_equal ScopedListItem.new(:todo_list_id => 7).scope_condition, + PreSuffixedScopedListItem.new(:todo_list_id => 7).scope_condition, + "acts_as_list.rb:35 appends `_id` only when the symbol does not " + + "already end in it. Doubling it would scope on a column that does " + + "not exist." + end + + # --------------------------------------------------------------------------- + # The condition in a real query -- what a String-scoped caller cannot prove + # --------------------------------------------------------------------------- + + test "positions are numbered within a scope, not across the table" do + first_in_one = ScopedListItem.create!(:todo_list_id => 1) + second_in_one = ScopedListItem.create!(:todo_list_id => 1) + first_in_two = ScopedListItem.create!(:todo_list_id => 2) + + # before_create -> add_to_list_bottom -> next_position_in_list -> bottom_item, + # which is where scope_condition reaches the database. + assert_equal 0, first_in_one.position + assert_equal 1, second_in_one.position + assert_equal 0, first_in_two.position, + "a second list must start its own numbering. Sharing one sequence " + + "means the scope was not applied." + end + + test "neighbour lookups do not cross the scope" do + top = ScopedListItem.create!(:todo_list_id => 1) + bottom = ScopedListItem.create!(:todo_list_id => 1) + other_list = ScopedListItem.create!(:todo_list_id => 2) + + assert_equal bottom, top.lower_item + assert_equal top, bottom.higher_item + assert_nil other_list.lower_item, + "other_list is alone in list 2. A neighbour here means the WHERE " + + "clause lost its scope and the whole table is one list." + assert_nil other_list.higher_item + end + + test "moving an item renumbers only its own list" do + top = ScopedListItem.create!(:todo_list_id => 1) + bottom = ScopedListItem.create!(:todo_list_id => 1) + untouched = ScopedListItem.create!(:todo_list_id => 2) + + bottom.move_to_top + + assert_equal 0, bottom.reload.position + assert_equal 1, top.reload.position + assert_equal 0, untouched.reload.position, + "increment_positions_on_higher_items runs an UPDATE built from " + + "scope_condition. An unscoped one would push list 2 down too." + end +end