Separating update into wizard and non wizard methods (#1713)

This continues work in preparation for #1684. Another step to moving the wizard into it's own controller
pulibrary · Mar 25, 2024 · 767aa88 · 767aa88
1 parent 8d22321
commit 767aa88
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 62 deletions.
diff --git a/app/controllers/works_controller.rb b/app/controllers/works_controller.rb
@@ -6,10 +6,10 @@
 # Currently this controller supports Multiple ways to create a work, wizard mode, create dataset, and migrate
 # The goal is to eventually break some of these workflows into separate contorllers.
 # For the moment I'm documenting which methods get called by each workflow below.
-# Note: new, edit and update get called by many of the workflows
+# Note: new, edit and update get called by both the migrate and Non wizard workflows
 #
 # wizard mode
-# new_sumission -> new_submission_save -> edit_wizard -> update -> readme_select -> readme_uploaded -> attachment_select ->
+# new_sumission -> new_submission_save -> edit_wizard -> update_wizard -> readme_select -> readme_uploaded -> attachment_select ->
 #     attachment_selected -> file_other ->                  review -> validate -> show & file_list
 #                         \> file_upload -> file_uploaded -^
 #
@@ -153,10 +153,32 @@ def edit
     end
   end
 
-  # Both wizard and not wizard mode
+  # PATCH /works/1/update-wizard
+  # only wizard  mode
+  def update_wizard
+    @work = Work.find(params[:id])
+    if handle_modification_permissions(uneditable_message: "Can not update work: #{@work.id} is not editable by #{current_user.uid}",
+                                       current_state_message: "Can not update work: #{@work.id} is not editable in current state by #{current_user.uid}")
+      work_before = @work.dup
+      if @work.update(update_params)
+        work_compare = WorkCompareService.new(work_before, @work)
+        @work.log_changes(work_compare, current_user.id)
+
+        redirect_to work_readme_select_url(@work)
+      else
+        # This is needed for rendering HTML views with validation errors
+        @form_resource_decorator = FormResourceDecorator.new(@work, current_user)
+
+        render :edit_wizard, status: :unprocessable_entity
+      end
+    end
+  end
+
+  # PATCH /works/1
+  # only non wizard mode
   def update
     @work = Work.find(params[:id])
-    if handle_modification_permissions(uneditiable_message: "Can not update work: #{@work.id} is not editable by #{current_user.uid}",
+    if handle_modification_permissions(uneditable_message: "Can not update work: #{@work.id} is not editable by #{current_user.uid}",
                                        current_state_message: "Can not update work: #{@work.id} is not editable in current state by #{current_user.uid}")
       update_work
     end
@@ -487,7 +509,6 @@ def wizard_mode?
     end
 
     def update_work
-      @wizard_mode = wizard_mode?
       upload_service = WorkUploadsEditService.new(@work, current_user)
       if @work.approved?
         upload_keys = deleted_files_param || []
@@ -537,15 +558,10 @@ def process_updates
         work_compare = WorkCompareService.new(work_before, @work)
         @work.log_changes(work_compare, current_user.id)
 
-        if @wizard_mode
-          redirect_to work_readme_select_url(@work)
-        else
-          redirect_to work_url(@work), notice: "Work was successfully updated."
-        end
+        redirect_to work_url(@work), notice: "Work was successfully updated."
       else
         # This is needed for rendering HTML views with validation errors
         @uploads = @work.uploads
-        @wizard_mode = wizard_mode?
         @form_resource_decorator = FormResourceDecorator.new(@work, current_user)
 
         render :edit, status: :unprocessable_entity
@@ -571,11 +587,11 @@ def migrated?
     end
 
     # @returns false if an error occured
-    def handle_modification_permissions(uneditiable_message: "Can not edit work: #{@work.id} is not editable by #{current_user.uid}",
+    def handle_modification_permissions(uneditable_message: "Can not edit work: #{@work.id} is not editable by #{current_user.uid}",
                                         current_state_message: "Can not edit work: #{@work.id} is not editable in current state by #{current_user.uid}")
       no_error = false
       if current_user.blank? || !@work.editable_by?(current_user)
-        Honeybadger.notify(uneditiable_message)
+        Honeybadger.notify(uneditable_message)
         redirect_to root_path, notice: I18n.t("works.uneditable.privs")
       elsif !@work.editable_in_current_state?(current_user)
         Honeybadger.notify(current_state_message)

diff --git a/app/views/works/_form.html.erb b/app/views/works/_form.html.erb
@@ -7,60 +7,35 @@
 -->
 <div class="d-flex align-items-start">
 
-  <!-- This div holds the "tabs menu" section that we display on the left -->
-  <div class="nav flex-column nav-pills me-3" id="v-pills-tab" role="tablist" aria-orientation="vertical">
-    <button class="nav-link active" id="v-pills-required-tab" data-bs-toggle="pill" data-bs-target="#v-pills-required" type="button" role="tab" aria-controls="v-pills-required" aria-selected="true">Required Metadata</button>
-    <button class="nav-link btn btn-light" id="v-pills-additional-tab" data-bs-toggle="pill" data-bs-target="#v-pills-additional" type="button" role="tab" aria-controls="v-pills-additional" aria-selected="false">Additional Metadata</button>
-    <button class="nav-link" id="v-pills-curator-controlled-tab" data-bs-toggle="pill" data-bs-target="#v-pills-curator-controlled" type="button" role="tab" aria-controls="v-pills-curator-controlled" aria-selected="false"><%= t('works.form.curator_controlled') %></button>
-  </div>
+<%= render 'form_nav' %>
 
   <div>
     <%= form_with(model: @work, class: ["work-form"], data: { work: @work.form_attributes }) do |form| %>
-      <!-- This div holds the actual tabs content (displayed on the right) -->
-      <div class="tab-content" id="v-pills-tabContent">
-
-        <!-- tab: required fields -->
-        <div class="tab-pane fade show active" id="v-pills-required" role="tabpanel" aria-labelledby="v-pills-required-tab">
-          <%= form.hidden_field :work_id %>
-          <%= render 'required_form' %>
-        </div>
+      <%= render partial: 'form_metadata', locals:  { form: form } %>
 
-        <!-- tab: additional fields -->
-        <div class="tab-pane fade" id="v-pills-additional" role="tabpanel" aria-labelledby="v-pills-additional-tab">
-          <%= render 'additional_form' %>
-        </div>
-
-        <!-- tab: curator controlled fields -->
-        <div class="tab-pane fade" id="v-pills-curator-controlled" role="tabpanel" aria-labelledby="v-pills-curator-controlled-tab">
-          <%= render 'curator_controlled_form', is_group_admin: current_user.has_role?(:group_admin, @work.group) %>
-        </div>
 
+      <hr />
+      <div class="deposit-uploads">
+        <% if @work.approved? %>
+          <section class="uploads">
+            <h2 class="h2"><%= t('works.uploads.post_curation.heading') %></h2>
+            <%= render(partial: 'works/s3_resources', locals: { edit_mode: false }) %>
+          </section>
+        <% else %>
+          <section class="uploads">
+            <h2 class="h2"><%= t('works.uploads.post_curation.heading') %></h2>
+            <%= render(partial: 'works/s3_resources', locals: { edit_mode: true, form: form }) %>
+          </section>
+          <div class="container-fluid deposit-uploads">
+            <div id="file-error" class="error_box"></div>
+            <%= form.label("Choose files to attach to this work", for: :pre_curation_uploads_added) %>
+            <%= form.file_field(:pre_curation_uploads_added, id: "pre_curation_uploads_added", multiple: true) %>
+            <!-- We populate this via JavaScript as the user selected files to upload -->
+            <div id="file-upload-list"></div>
+          </div>
+        <% end %>
       </div>
 
-      <% if !@wizard_mode %>
-        <hr />
-        <div class="deposit-uploads">
-          <% if @work.approved? %>
-            <section class="uploads">
-              <h2 class="h2"><%= t('works.uploads.post_curation.heading') %></h2>
-              <%= render(partial: 'works/s3_resources', locals: { edit_mode: false }) %>
-            </section>
-          <% else %>
-            <section class="uploads">
-              <h2 class="h2"><%= t('works.uploads.post_curation.heading') %></h2>
-              <%= render(partial: 'works/s3_resources', locals: { edit_mode: true, form: form }) %>
-            </section>
-            <div class="container-fluid deposit-uploads">
-              <div id="file-error" class="error_box"></div>
-              <%= form.label("Choose files to attach to this work", for: :pre_curation_uploads_added) %>
-              <%= form.file_field(:pre_curation_uploads_added, id: "pre_curation_uploads_added", multiple: true) %>
-              <!-- We populate this via JavaScript as the user selected files to upload -->
-              <div id="file-upload-list"></div>
-            </div>
-          <% end %>
-        </div>
-      <% end %>
-
       <hr />
       <div class="actions">
         <%= link_to 'Cancel', @work, class: "btn btn-secondary" %>

diff --git a/app/views/works/_form_metadata.html.erb b/app/views/works/_form_metadata.html.erb
@@ -0,0 +1,20 @@
+<!-- This div holds the actual tabs content (displayed on the right) -->
+<div class="tab-content" id="v-pills-tabContent">
+
+    <!-- tab: required fields -->
+    <div class="tab-pane fade show active" id="v-pills-required" role="tabpanel" aria-labelledby="v-pills-required-tab">
+    <%= form.hidden_field :work_id %>
+    <%= render 'required_form' %>
+    </div>
+
+    <!-- tab: additional fields -->
+    <div class="tab-pane fade" id="v-pills-additional" role="tabpanel" aria-labelledby="v-pills-additional-tab">
+    <%= render 'additional_form' %>
+    </div>
+
+    <!-- tab: curator controlled fields -->
+    <div class="tab-pane fade" id="v-pills-curator-controlled" role="tabpanel" aria-labelledby="v-pills-curator-controlled-tab">
+    <%= render 'curator_controlled_form', is_group_admin: current_user.has_role?(:group_admin, @work.group) %>
+    </div>
+
+</div>
diff --git a/app/views/works/_form_nav.html.erb b/app/views/works/_form_nav.html.erb
@@ -0,0 +1,6 @@
+<!-- This div holds the "tabs menu" section that we display on the left -->
+<div class="nav flex-column nav-pills me-3" id="v-pills-tab" role="tablist" aria-orientation="vertical">
+  <button class="nav-link active" id="v-pills-required-tab" data-bs-toggle="pill" data-bs-target="#v-pills-required" type="button" role="tab" aria-controls="v-pills-required" aria-selected="true">Required Metadata</button>
+  <button class="nav-link btn btn-light" id="v-pills-additional-tab" data-bs-toggle="pill" data-bs-target="#v-pills-additional" type="button" role="tab" aria-controls="v-pills-additional" aria-selected="false">Additional Metadata</button>
+  <button class="nav-link" id="v-pills-curator-controlled-tab" data-bs-toggle="pill" data-bs-target="#v-pills-curator-controlled" type="button" role="tab" aria-controls="v-pills-curator-controlled" aria-selected="false"><%= t('works.form.curator_controlled') %></button>
+</div>
diff --git a/app/views/works/_form_wizard.html.erb b/app/views/works/_form_wizard.html.erb
@@ -0,0 +1,29 @@
+<%= render 'form_errors' %>
+
+<!--
+  Information about Bootstrap tabs
+  https://getbootstrap.com/docs/5.0/components/navs-tabs/
+  https://getbootstrap.com/docs/5.0/components/navs-tabs/#javascript-behavior
+-->
+<div class="d-flex align-items-start">
+
+<%= render 'form_nav' %>
+
+  <div>
+    <%= form_with(model: @work, url: update_work_wizard_path(@work), class: ["work-form"], data: { work: @work.form_attributes }) do |form| %>
+      <%= render partial: 'form_metadata', locals:  { form: form } %>
+
+      <hr />
+      <div class="actions">
+        <%= link_to 'Cancel', @work, class: "btn btn-secondary" %>
+        <% if @work.persisted? %>
+          <%= form.submit "Save Work", class: "btn btn-primary wizard-next-button", id: "btn-submit" %>
+        <% else %>
+          <%= form.submit "Create", class: "btn btn-primary wizard-next-button", id: "btn-submit", name: "submit" %>
+        <% end %>
+      </div>
+
+      <%= render 'form_hidden_fields' %>
+    <% end %>
+  </div>
+</div>
diff --git a/app/views/works/edit_wizard.html.erb b/app/views/works/edit_wizard.html.erb
@@ -1,7 +1,7 @@
 <div class="wizard-area">
   <h1>New Submission</h1>
   <%= render "wizard_progress", wizard_step: 1 %>
-  <%= render 'form' %>
+  <%= render 'form_wizard' %>
 </div>
 
 <%= javascript_include_tag 'edit_work_utils' %>
diff --git a/config/routes.rb b/config/routes.rb
@@ -36,6 +36,7 @@
   get "works/:id/attachment-select", to: "works#attachment_select", as: :work_attachment_select
   post "works/:id/attachment-select", to: "works#attachment_selected", as: :work_attachment_selected
   patch "works/:id/file-upload", to: "works#file_uploaded", as: :work_file_uploaded
+  patch "works/:id/update-wizard", to: "works#update_wizard", as: :update_work_wizard
   get "works/:id/edit-wizard", to: "works#edit_wizard", as: :edit_work_wizard
   get "works/:id/file-upload", to: "works#file_upload", as: :work_file_upload
   get "works/:id/file-other", to: "works#file_other", as: :work_file_other

diff --git a/spec/controllers/works_controller_spec.rb b/spec/controllers/works_controller_spec.rb
@@ -129,7 +129,7 @@
           creators: [{ "orcid" => "", "given_name" => "Jane", "family_name" => "Smith" }]
         }
         sign_in user
-        post(:update, params:)
+        post(:update_wizard, params:)
         expect(response.status).to be 302
         expect(response.location).to eq "http://test.host/works/#{work.id}/readme-select"
         expect(ActiveStorage::PurgeJob).not_to have_received(:new)

diff --git a/spec/system/work_create_spec.rb b/spec/system/work_create_spec.rb
@@ -228,6 +228,8 @@
           expect(work.resource.related_objects.count).to eq(0)
           # rubocop:disable Layout/LineLength
           expect(page).to have_content("1 error prohibited this dataset from being saved:\nRelated Objects are invalid: Related Identifier Type is missing or invalid for https://related.example.com, Relationship Type is missing or invalid for https://related.example.com")
+          expect(page).not_to have_content("Uncurated Files")
+          expect(page).not_to have_content("Curated Files")
           # rubocop:enable Layout/LineLength
         end
       end