From 1aae6dbf9aeb4a4d4cb0470e5e621d30f031064f Mon Sep 17 00:00:00 2001
From: Finn Bacall <finn.bacall@manchester.ac.uk>
Date: Thu, 14 Sep 2023 14:45:02 +0100
Subject: [PATCH] Render batch change results in partial instead of `flash`

Avoid some exception use
---
 .../batch_sharing_permission_preview.html.erb |  1 +
 app/views/isa_studies/_buttons.html.erb       |  4 +++
 .../sharing/_batch_change_results.html.erb    | 36 +++++++++++++++++++
 ...batch_sharing_permissions_changed.html.erb | 18 ++--------
 lib/seek/publishing/publishing_common.rb      |  4 +--
 lib/seek/sharing/sharing_common.rb            | 28 +++++----------
 6 files changed, 52 insertions(+), 39 deletions(-)
 create mode 100644 app/views/sharing/_batch_change_results.html.erb

diff --git a/app/views/assets/sharing/batch_sharing_permission_preview.html.erb b/app/views/assets/sharing/batch_sharing_permission_preview.html.erb
index 28d6ed083e..30d8807f0b 100644
--- a/app/views/assets/sharing/batch_sharing_permission_preview.html.erb
+++ b/app/views/assets/sharing/batch_sharing_permission_preview.html.erb
@@ -3,6 +3,7 @@
   @policy_params = params[:policy_attributes]
 
 %>
+<%= render partial: 'sharing/batch_change_results' %>
 
 <%= show_title "Change permissions of items related to #{link_to(h(current_user.person.name),current_user.person)} in batch".html_safe -%>
 
diff --git a/app/views/isa_studies/_buttons.html.erb b/app/views/isa_studies/_buttons.html.erb
index 8aee3c3300..5a0e60399a 100644
--- a/app/views/isa_studies/_buttons.html.erb
+++ b/app/views/isa_studies/_buttons.html.erb
@@ -21,6 +21,10 @@ exportToExcel||="sampleExportExcel()"
   function loadBatchPermission(dtName, e){
     const sample_ids = window[dtName].selectedSampleIds()
     let publish_list = {'Sample': {}}
+    if (!sample_ids.length) {
+        alert('Please select at least one sample.');
+        return;
+    }
     for(let id of sample_ids){ publish_list['Sample'][id]='1' }
 
     $j.ajax({
diff --git a/app/views/sharing/_batch_change_results.html.erb b/app/views/sharing/_batch_change_results.html.erb
new file mode 100644
index 0000000000..8ba1c01693
--- /dev/null
+++ b/app/views/sharing/_batch_change_results.html.erb
@@ -0,0 +1,36 @@
+<% return unless @items_for_sharing %>
+<% items_text = 'item'.pluralize(@items_for_sharing.size) %>
+<% if @success&.any? || @gatekeeper_required&.any? %>
+  <div id="notice_flash" class="fade in alert alert-success alert-dismissable" role="alert">
+    <% if @success.any? %>
+      The sharing policies for your selected <%= items_text -%> were successfully updated:
+      <ul id="ok">
+        <% @success.each do |item| %>
+          <li><%= item.title %></li>
+        <% end %>
+      </ul>
+    <% end %>
+
+    <% if @gatekeeper_required.any? %>
+      Publishing the following <%= items_text -%> requires approval from a gatekeeper:
+      <ul id="gk">
+        <% @gatekeeper_required.each do |item| %>
+          <li><%= item.title %></li>
+        <% end %>
+      </ul>
+      The <%= items_text -%> will not be published until approved.
+    <% end %>
+  </div>
+<% elsif @error&.any? %>
+  <div id="notice_flash" class="fade in alert alert-danger alert-dismissable" role="alert">
+    The sharing policies for your selected <%= items_text -%> were not successfully updated:
+    <ul>
+      <% @error.each do |item| %>
+        <li>
+          <%= item.title %><br>
+          The reason: <%= item.errors.full_messages.join(', ') %>
+        </li>
+      <% end %>
+    </ul>
+  </div>
+<% end %>
diff --git a/app/views/single_pages/sample_batch_sharing_permissions_changed.html.erb b/app/views/single_pages/sample_batch_sharing_permissions_changed.html.erb
index 0bab165b05..0872607cfd 100644
--- a/app/views/single_pages/sample_batch_sharing_permissions_changed.html.erb
+++ b/app/views/single_pages/sample_batch_sharing_permissions_changed.html.erb
@@ -1,17 +1,3 @@
-<%
-  flash_exists = flash[:error] || flash[:notice]
-%>
-
-<% if flash_exists %>
-  <div id="notice_flash" class="fade in alert alert-<%= flash[:notice] ? "success" : "danger" %> alert-dismissable" role="alert">
-    <% if flash[:notice] %>
-      <%= flash[:notice]%>
-    <% else%>
-      <%= flash[:error]%>
-      <% flash[:error]=nil %>
-    <% end %>
-  </div>
-    <a id="hide-permissions-changed" class=" btn btn-default" onclick="$j('#change-batch-permission-modal').modal('hide')">Ok</a>
-<% end %>
-
+<%= render partial: 'sharing/batch_change_results' %>
 
+<a id="hide-permissions-changed" class=" btn btn-default" onclick="$j('#change-batch-permission-modal').modal('hide')">OK</a>
diff --git a/lib/seek/publishing/publishing_common.rb b/lib/seek/publishing/publishing_common.rb
index 57104602be..1ce8e6ab71 100644
--- a/lib/seek/publishing/publishing_common.rb
+++ b/lib/seek/publishing/publishing_common.rb
@@ -142,7 +142,7 @@ def set_investigations
         @assets.each do |type, klass|
           next if %w[Investigation Study Assay].include? type
           klass.each do |asset|
-            if asset.investigations.empty?
+            if !asset.respond_to?(:investigations) || asset.investigations.empty?
               @assets_not_in_isa.push(asset)
             else
               asset.investigations.each do |inv|
@@ -150,8 +150,6 @@ def set_investigations
                 @investigations.push(inv)
               end
             end
-            rescue NoMethodError
-              @assets_not_in_isa.push(asset)
           end
         end
       end
diff --git a/lib/seek/sharing/sharing_common.rb b/lib/seek/sharing/sharing_common.rb
index 300e92e333..f18426e7f0 100644
--- a/lib/seek/sharing/sharing_common.rb
+++ b/lib/seek/sharing/sharing_common.rb
@@ -37,35 +37,23 @@ def batch_sharing_permission_preview
       def batch_sharing_permission_changed
         @items_for_sharing = resolve_sharing_params(params[:publish])
         @batch_sharing_permission_changed = true
-        notice_count = 0
-        gatekeeper_count = 0
-        error_count = 0
-        flash[:notice] = "The sharing policies for your selected #{"item".pluralize(@items_for_sharing.size)} were successfully updated:<ul id=\"ok\"> "
-        gatekeeper_flash = "Publishing the following #{"item".pluralize(@items_for_sharing.size)} requires approval from a gatekeeper:<ul id=\"gk\"> "
-        flash[:error] = "The sharing policies for your selected #{"item".pluralize(@items_for_sharing.size)} were not successfully updated:<ul> "
+        @success = []
+        @gatekeeper_required = []
+        @error = []
         @items_for_sharing.each do |item|
           item.policy.update_with_bulk_sharing_policy(policy_params) if policy_params.present?
-          begin
-            item.save!
+          if item.save
             request_publish_approval_batch_sharing(item)  # Has to go before log_publishing_batch_sharing(item)
             log_publishing_batch_sharing(item)
             if item.is_waiting_approval? && is_gatekeeper_approval_required?(item)
-              gatekeeper_count += 1
-              gatekeeper_flash += "<li>#{item.title}</li>"
+              @gatekeeper_required << item
             else
-              notice_count += 1
-              flash[:notice] += "<li>#{item.title}</li>"
+              @success << item
             end
-          rescue Exception => e
-            error_count += 1
-            flash[:error] += "<li>#{item.title}<br>"
-            flash[:error] += "The reason:  #{e.message}</li>"
+          else
+            @error << item
           end
         end
-        flash[:notice] = notice_count == 0 ? "": (flash[:notice]+"</ul>").html_safe
-        flash[:notice] += gatekeeper_count == 0 ? "": (gatekeeper_flash+"</ul>"+"The #{"item".pluralize(gatekeeper_count)} will not be published until approved.").html_safe
-        flash.now[:notice] = (notice_count+gatekeeper_count) == 0 ? nil: flash[:notice].html_safe
-        flash.now[:error] = error_count == 0 ? nil: (flash[:error]+"</ul>").html_safe
         if params[:single_page]
           render 'single_pages/sample_batch_sharing_permissions_changed', { layout: false }
         else