Refactor addAuthor (JavaScript) for clarity and maintainability

app/assets/javascripts/add_remove_authors.js

@@ -1,25 +1,134 @@
-function addAuthor(type, count)    {
-  var count = count || 1;
-  var newAuthor = document.createElement("div");
-  var firstI = document.createElement("div");
-  var lastI = document.createElement("div");
-  var deleteB = document.createElement("div");
-  newAuthor.setAttribute("class", "form-row");
-  firstI.setAttribute("class", "col-md-5");
-  lastI.setAttribute("class", "col-md-5");
-  deleteB.setAttribute("class", "col-md-2");
-  firstI.innerHTML = `<input type="text" name="${type}[author_first_name][]" id="${type}_author_first_name_" required="required" class="form-control form-group">`;
-  lastI.innerHTML = `<input type="text" name="${type}[author_last_name][]" id="${type}_author_last_name_" required="required" class="form-control form-group">`;
-  deleteB.innerHTML = `<button name="button" type="button" onclick="removeAuthor('added_${count}');" class="form-control form-group bg-danger text-white">Remove Author</button>`;
-  newAuthor.setAttribute("id", "added_" + count);
-  newAuthor.appendChild(firstI);
-  newAuthor.appendChild(lastI);
-  newAuthor.appendChild(deleteB);
-  document.getElementById("author_group").appendChild(newAuthor);
-  count++;
-  document.getElementById("add_author_btn").setAttribute("onClick", `addAuthor('${type}', ${count})`);
-}
-
-function removeAuthor(idToDelete)     {
-  document.getElementById(idToDelete).outerHTML = '';
+
+// Add an event listener to the author group element that will allow the "Remove Author" button to work
+// even if the author group element is dynamically added to the page.
+document.addEventListener("turbolinks:load", function() {
+    const authorGroupElement = document.getElementById('author_group');
+
+    if (authorGroupElement) {
+        authorGroupElement.addEventListener('click', function(event) {
+            if (event.target &&
+                event.target.matches("button[type='button']") &&
+                ['Remove Author', 'Remove Artist'].includes(event.target.textContent)
+                ) {
+                // Navigate up two levels to get the grandparent element, which is the author element.
+                const authorElement = event.target.parentNode.parentNode;
+                const authorElements = Array.from(authorGroupElement.children);
+                const authorIndex = authorElements.indexOf(authorElement);
+
+                if (authorIndex !== -1) {
+                    removeAuthor(authorIndex);
+                    updateAuthorIds()
+                }
+            }
+        });
+    }
+});
+
+function getRoleFromButton() {
+    const button = document.getElementById('add_author_btn');
+    if (!button) return null; // Return null if the button doesn't exist
+
+    return button.textContent.includes('Artist') ? 'Artist' : 'Author';
+}
+
+function removeAuthor(authorIndex) {
+    const authorGroupElement = document.getElementById('author_group');
+    if (authorGroupElement && authorGroupElement.children[authorIndex]) {
+        authorGroupElement.children[authorIndex].remove();
+        updateAuthorIds();
+    }
+}
+
+
+function addAuthor(type) {
+    const authorGroup = document.getElementById("author_group");
+    if (!authorGroup) return;
+
+    const newAuthor = createAuthorElement(type);
+    authorGroup.appendChild(newAuthor);
+    updateAuthorIds();
+}
+
+function createAuthorElement(type) {
+    const newDataIndex = document.querySelectorAll(`[data-type='${type}']`).length;
+    const newAuthor = createElementWithAttributes("div", { class: "form-row", 'data-type': type, 'data-index': newDataIndex });
+    newAuthor.appendChild(createInput(type, "author_first_name"));
+    newAuthor.appendChild(createInput(type, "author_last_name"));
+    newAuthor.appendChild(createDeleteButton());
+    return newAuthor;
+}
+
+function createElementWithAttributes(tag, attributes) {
+    const element = document.createElement(tag);
+    Object.entries(attributes).forEach(([key, value]) => element.setAttribute(key, value));
+    return element;
+}
+
+function createInput(type, name) {
+    // The "for" attribute of the label will be updated with the updateAuthorIds function.
+
+    const role = getRoleFromButton();
+    if (!role) return;
+
+    const wrapper = createElementWithAttributes("div", {
+        class: "col-md-5",
+        "data-type": type
+    });
+
+    const label = createElementWithAttributes("label", {
+        class: "required",
+        "data-type": type
+    });
+
+    label.textContent = name.includes('first_name') ? role + ' First Name' : role + ' Last Name';
+
+    const input = createElementWithAttributes("input", {
+        type: "text",
+        name: `${type}[${name}][]`,
+        required: "required",
+        class: "form-control form-group",
+        "data-type": type
+    });
+
+    wrapper.appendChild(label);
+    wrapper.appendChild(input);
+
+    return wrapper;
 }
+
+function createDeleteButton() {
+    const role = getRoleFromButton();
+    if (!role) return;
+
+    const wrapper = createElementWithAttributes("div", { class: "col-md-2" });
+    const button = createElementWithAttributes("button", {
+        type: "button",
+        class: "form-control form-group bg-danger text-white"
+    });
+    button.textContent = "Remove " + role;
+    wrapper.appendChild(button);
+    return wrapper;
+}
+
+function updateAuthorIds() {
+    const authorGroups = document.querySelectorAll('#author_group .form-row');
+    authorGroups.forEach((group, index) => {
+        group.id = `author_${index}`;
+        updateFieldAndLabel(group, 'first_name', index);
+        updateFieldAndLabel(group, 'last_name', index);
+    });
+}
+
+function updateFieldAndLabel(group, fieldName, index) {
+    const type = group.getAttribute('data-type');
+    if (!type) return;
+
+    const input = group.querySelector(`[name='${type}[author_${fieldName}][]']`);
+    if (!input) return;
+
+    input.id = `author_${fieldName}_${index}`;
+    const label = input.previousElementSibling;
+    if (label && label.tagName === 'LABEL') {
+        label.setAttribute('for', input.id);
+    }
+}

app/views/partials/_author.html.erb

@@ -1,26 +1,27 @@
 
 <div id="author_group">
-    <% if publication.author_first_name.empty? && publication.author_last_name.empty? %>
+  <% if publication.author_first_name.empty? && publication.author_last_name.empty? %>
     <% publication.author_first_name << '' %>
     <% publication.author_last_name << '' %>
-    <% end %>
+  <% end %>
 
-    <% publication.author_first_name.each_with_index do |_, index| %>
-    <div class="form-row" id="author_<%= index %>">
-        <div class="col-md-5">
-            <%= label_tag(:author_first_name, author_or_artist_label + ' First Name', class: "required") %>
-            <%= text_field_tag "#{name}[author_first_name][]", publication.author_first_name[index], required: true, class: "form-control form-group" %>
-        </div>
-        <div class="col-md-5">
-        <%= label_tag "#{name}_author_last_name_#{index}", "Author Last Name", class: "required" %>
-        <%= text_field_tag "#{name}[author_last_name][]", publication.author_last_name[index], required: true, class: "form-control form-group" %>
-        </div>
-        <% if index > 0 %>
+  <% publication.author_first_name.zip(publication.author_last_name).each_with_index do |(first_name, last_name), index| %>
+    <div class="form-row" data-type="<%= name %>" data-index=<%= index %> id="author_<%= index %>">
+      <div class="col-md-5">
+        <%= label_tag("author_first_name_#{index}", "#{author_or_artist_label} First Name", class: "required") %>
+        <%= text_field_tag "#{name}[author_first_name][]", first_name, "data-publication-type": name, id: "author_first_name_#{index}", required: true, class: "form-control form-group" %>
+      </div>
+      <div class="col-md-5">
+        <%= label_tag "author_last_name_#{index}", "#{author_or_artist_label} Last Name", class: "required" %>
+        <%= text_field_tag "#{name}[author_last_name][]", last_name, "data-publication-type": name, id: "author_last_name_#{index}", required: true, class: "form-control form-group" %>
+      </div>
+      <% if index > 0 %>
         <div class="col-md-2">
-            <%= button_tag "Remove Author", type: "button", onclick: "removeAuthor('author_#{index}')", class: "form-control form-group bg-danger text-white" %>
+          <%= button_tag "Remove " + author_or_artist_label, type: "button", class: "form-control form-group bg-danger text-white" %>
         </div>
-        <% end %>
+      <% end %>
     </div>
-    <% end %>
+  <% end %>
 </div>
-<%= button_tag "Add Author", type: "button", id: "add_author_btn", onclick: "addAuthor('#{name}')", class: "btn btn-primary" %>
+
+<%= button_tag "Add " + author_or_artist_label, type: "button", id: "add_author_btn", onclick: "addAuthor('#{name}')", class: "btn btn-primary" %>

app/views/partials/_publications_colleges.html.erb

@@ -1,13 +1,16 @@
 <div class="form-group col-md-6" id="colleges-group">
-  <%= form.label(:college_ids, author_or_artist_label + " College(s)") %><br />
-  <%= form.collection_check_boxes :college_ids, College.all, :id, :name do |c| %>
-    <div class="form-check">
-      <%= c.check_box class: "form-check-input" %>
-      <%= c.label class: "form-check-label" %>
-    </div>
-  <% end %>
+  <fieldset>
+    <legend><%= author_or_artist_label %> College(s)</legend><br/>
+    <%= form.collection_check_boxes :college_ids, College.all, :id, :name do |b| %>
+      <div class="form-check">
+        <%= b.check_box class: "form-check-input" %>
+        <%= b.label class: "form-check-label" %>
+      </div>
+    <% end %>
+  </fieldset>
 </div>
 
+
 <div class="form-row">
   <div id="other_college_group" class="form-group col-md-6">
     <%= form.label :other_college, 'Other College' %>

spec/features/author_management/adding_authors_spec.rb

@@ -0,0 +1,127 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Adding Authors', :feature, js: true do
+  let(:submitter) { FactoryBot.create(:submitter) }
+
+  before do
+    create_submitter(submitter)
+  end
+
+  it 'adds authors to a new publication' do
+    visit new_other_publication_path
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.new_other_publication_path)
+
+    # Verify blank input fields for author's first name and last name
+    # to be present on page load
+    expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 1)
+    expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 1)
+    check_field_values_by_index(0, '', '')
+
+    # Fill out the fields with the first author's name
+    first_name_fields.last.set('First0')
+    last_name_fields.last.set('Last0')
+
+    # Click "Add Author" and verify new and old fields
+    click_on 'Add Author'
+
+    expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 2)
+    expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 2)
+
+    check_field_values_by_index(0, 'First0', 'Last0')
+    check_field_values_by_index(1, '', '')
+
+    # Fill in second author's name
+    first_name_fields.last.set('First1')
+    last_name_fields.last.set('Last1')
+
+    # Click "Add Author" again
+    click_on 'Add Author'
+    expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 3)
+    expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 3)
+    check_field_values_by_index(0, 'First0', 'Last0')
+    check_field_values_by_index(1, 'First1', 'Last1')
+    check_field_values_by_index(2, '', '')
+
+    # Fill in third author's name
+    first_name_fields.last.set('First2')
+    last_name_fields.last.set('Last2')
+
+    # Click "Add Author" again
+    click_on 'Add Author'
+    expect(page).to have_selector("input[name='other_publication[author_first_name][]']", count: 4)
+    expect(page).to have_selector("input[name='other_publication[author_last_name][]']", count: 4)
+    check_field_values_by_index(0, 'First0', 'Last0')
+    check_field_values_by_index(1, 'First1', 'Last1')
+    check_field_values_by_index(2, 'First2', 'Last2')
+    check_field_values_by_index(3, '', '')
+
+    # Fill in fourth author's name
+    first_name_fields.last.set('First3')
+    last_name_fields.last.set('Last3')
+
+    # Fill in the rest of the fields
+    fill_in 'other_publication[work_title]', with: 'Title'
+    fill_in 'other_publication[other_title]', with: 'Subtitle'
+    fill_in 'other_publication[uc_department]', with: 'Department'
+    fill_in 'other_publication[publication_date]', with: 'Date'
+    fill_in 'other_publication[url]', with: 'URL'
+    fill_in 'other_publication[doi]', with: 'DOI'
+
+    # Click "Submit" and verify that we are redirected to the index page
+    # and that a success message is displayed
+    click_on 'Submit'
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.publications_path)
+    expect(page).to have_text 'Other Publication was successfully created.'
+
+    # Click on the hyperlink on the id of the newly created publication
+    # and verify that the author names are correct
+    click_on OtherPublication.last.work_title.to_s
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
+    expect(page).to have_text 'First0 Last0'
+    expect(page).to have_text 'First1 Last1'
+    expect(page).to have_text 'First2 Last2'
+    expect(page).to have_text 'First3 Last3'
+  end
+
+  it 'adds authors to an existing publication' do
+    # Create a new publication.  Adding author functionality for a new publication
+    # is tested in the previous test.
+    create_other_publication # Defined in spec/support/helpers/feature_spec_helpers/author_management.rb
+
+    # Click on the hyperlink on the id of the newly created publication
+    # and verify that the author names are correct
+    click_on OtherPublication.last.work_title.to_s
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
+    expect(page).to have_selector('td', text: 'First0 Last0') # Information is in table format on the show page
+
+    # Click on "Edit" and verify that we are redirected to the edit page
+    # and that the author names are correct
+    click_on 'Edit'
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.edit_other_publication_path(OtherPublication.last.id))
+    check_field_values_by_index(0, 'First0', 'Last0')
+
+    # Add another author and verify that the author names are correct
+    click_on 'Add Author'
+    first_name_fields.last.set('First1')
+    last_name_fields.last.set('Last1')
+    check_field_values_by_index(0, 'First0', 'Last0')
+    check_field_values_by_index(1, 'First1', 'Last1')
+
+    # Add a third author and verify that the author names are correct
+    click_on 'Add Author'
+    first_name_fields.last.set('First2')
+    last_name_fields.last.set('Last2')
+    check_field_values_by_index(0, 'First0', 'Last0')
+    check_field_values_by_index(1, 'First1', 'Last1')
+    check_field_values_by_index(2, 'First2', 'Last2')
+
+    # Save the changes and verify that we are redirected to the show page
+    # and that the author names are correct
+    click_on 'Submit'
+    expect(page).to have_current_path(Rails.application.routes.url_helpers.other_publication_path(OtherPublication.last.id))
+    expect(page).to have_text 'Other Publication was successfully updated.'
+    expect(page).to have_selector('td', text: 'First0 Last0, First1 Last1, First2 Last2') # Information is in table format on the show page
+  end
+end