Bug/59065 error toast giving a 500 error without relevant details (#1…

…7207)

https://community.openproject.org/work_packages/59065

app/controllers/work_packages/activities_tab_controller.rb

@@ -106,28 +106,33 @@ def cancel_edit
   end
 
   def create
-    call = create_journal_service_call
+    begin
+      call = create_journal_service_call
 
-    if call.success? && call.result
-      set_last_server_timestamp_to_headers
-      handle_successful_create_call(call)
-    else
-      handle_failed_create_call(call) # errors should be rendered in the form
-      @turbo_status = :bad_request
+      if call.success? && call.result
+        set_last_server_timestamp_to_headers
+        handle_successful_create_call(call)
+      else
+        handle_failed_create_or_update_call(call)
+      end
+    rescue StandardError => e
+      handle_internal_server_error(e)
     end
 
     respond_with_turbo_streams
   end
 
   def update
-    call = Journals::UpdateService.new(model: @journal, user: User.current).call(
-      notes: journal_params[:notes]
-    )
+    begin
+      call = update_journal_service_call
 
-    if call.success? && call.result
-      update_item_show_component(journal: call.result, grouped_emoji_reactions: grouped_emoji_reactions_for_journal)
-    else
-      handle_failed_update_call(call)
+      if call.success? && call.result
+        update_item_show_component(journal: call.result, grouped_emoji_reactions: grouped_emoji_reactions_for_journal)
+      else
+        handle_failed_create_or_update_call(call)
+      end
+    rescue StandardError => e
+      handle_internal_server_error(e)
     end
 
     respond_with_turbo_streams
@@ -182,19 +187,11 @@ def respond_with_error(error_message)
       # turbo_stream requests (tab is already rendered and an error occured in subsequent requests) are handled below
       format.turbo_stream do
         @turbo_status = :not_found
-        render_error_banner_via_turbo_stream(error_message)
+        render_error_flash_message_via_turbo_stream(message: error_message)
       end
     end
   end
 
-  def render_error_banner_via_turbo_stream(error_message)
-    update_via_turbo_stream(
-      component: WorkPackages::ActivitiesTab::ErrorStreamComponent.new(
-        error_message:
-      )
-    )
-  end
-
   def find_work_package
     @work_package = WorkPackage.find(params[:work_package_id])
   rescue ActiveRecord::RecordNotFound
@@ -259,22 +256,22 @@ def perform_update_streams_from_last_update_timestamp
     end
   end
 
-  def handle_failed_create_call(call)
-    update_via_turbo_stream(
-      component: WorkPackages::ActivitiesTab::Journals::NewComponent.new(
-        work_package: @work_package,
-        journal: call.result,
-        form_hidden_initially: false
-      )
-    )
-  end
-
-  def handle_failed_update_call(call)
+  def handle_failed_create_or_update_call(call)
     @turbo_status = if call.errors&.first&.type == :error_unauthorized
                       :forbidden
                     else
                       :bad_request
                     end
+    render_error_flash_message_via_turbo_stream(
+      message: call.errors&.full_messages&.join(", ")
+    )
+  end
+
+  def handle_internal_server_error(error)
+    @turbo_status = :internal_server_error
+    render_error_flash_message_via_turbo_stream(
+      message: error.message
+    )
   end
 
   def replace_whole_tab
@@ -306,6 +303,12 @@ def create_journal_service_call
     ###
   end
 
+  def update_journal_service_call
+    Journals::UpdateService.new(model: @journal, user: User.current).call(
+      notes: journal_params[:notes]
+    )
+  end
+
   def generate_time_based_update_streams(last_update_timestamp)
     journals = @work_package
                  .journals

frontend/src/app/core/turbo/turbo-requests.service.ts

@@ -13,19 +13,26 @@ export class TurboRequestsService {
   public request(url:string, init:RequestInit = {}, suppressErrorToast = false):Promise<{ html:string, headers:Headers }> {
     return fetch(url, init)
       .then((response) => {
-        if (!response.ok) {
-          throw new Error(`HTTP ${response.status}: ${response.statusText}`);
-        }
         return response.text().then((html) => ({
           html,
           headers: response.headers,
+          response,
         }));
       })
       .then((result) => {
+        // the result may contain a primer error banner if any server side error appeared
+        // thus we need to render the html even for non-ok responses
         renderStreamMessage(result.html);
-        return result;
+        // after rendering the html, check if the response and throw an error if it's not ok
+        if (!result.response.ok) {
+          throw new Error(result.response.statusText);
+        } else {
+          // enable further processing of the html and headers in the calling function
+          return { html: result.html, headers: result.headers };
+        }
       })
       .catch((error) => {
+        // this should only catch errors happening in the client side parsing in the above .then() calls
         if (!suppressErrorToast) {
           this.toast.addError(error as string);
         } else {

frontend/src/stimulus/controllers/dynamic/work-packages/activities-tab/index.controller.ts

@@ -637,7 +637,7 @@ export default class IndexController extends Controller {
       headers: {
         'X-CSRF-Token': (document.querySelector('meta[name="csrf-token"]') as HTMLMetaElement).content,
       },
-    });
+    }, true);
   }
 
   private handleSuccessfulSubmission(html:string, headers:Headers) {

spec/features/activities/work_package/activities_spec.rb

@@ -27,8 +27,11 @@
 #++
 
 require "spec_helper"
+require "support/flash/expectations"
 
 RSpec.describe "Work package activity", :js, :with_cuprite, with_flag: { primerized_work_package_activities: true } do
+  include Flash::Expectations
+
   let(:project) { create(:project) }
   let(:admin) { create(:admin) }
   let(:member_role) do
@@ -1244,4 +1247,111 @@
       end
     end
   end
+
+  describe "error handling" do
+    let(:work_package) { create(:work_package, project:, author: admin) }
+
+    current_user { admin }
+
+    before do
+      wp_page.visit!
+      wp_page.wait_for_activity_tab
+    end
+
+    context "when adding a comment" do
+      context "when the creation call raises an unknown server error" do
+        before do
+          allow_any_instance_of(WorkPackages::ActivitiesTabController) # rubocop:disable RSpec/AnyInstance
+            .to receive(:create_journal_service_call)
+            .and_raise(StandardError.new("Test error"))
+        end
+
+        it "shows an error banner when the server returns an error" do
+          activity_tab.add_comment(text: "First comment by admin", save: false)
+
+          page.find_test_selector("op-submit-work-package-journal-form").click
+
+          expect_flash(message: "Test error", type: :error)
+
+          # expect the editor content not to be lost
+          within_test_selector("op-work-package-journal-form-element") do
+            editor = FormFields::Primerized::EditorFormField.new("notes", selector: "#work-package-journal-form-element")
+            editor.expect_value("First comment by admin")
+          end
+        end
+      end
+
+      context "when the creation call fails with a validation error" do
+        before do
+          allow_any_instance_of(AddWorkPackageNoteService) # rubocop:disable RSpec/AnyInstance
+            .to receive(:call)
+            .and_return(
+              ServiceResult.failure(errors: ActiveModel::Errors.new(Journal.new).tap do |e|
+                e.add(:notes, "Validation error")
+              end)
+            )
+        end
+
+        it "shows a validation error banner" do
+          activity_tab.add_comment(text: "First comment by admin", save: false)
+
+          page.find_test_selector("op-submit-work-package-journal-form").click
+
+          expect_flash(message: "Validation error", type: :error)
+
+          # expect the editor content not to be lost
+          within_test_selector("op-work-package-journal-form-element") do
+            editor = FormFields::Primerized::EditorFormField.new("notes", selector: "#work-package-journal-form-element")
+            editor.expect_value("First comment by admin")
+          end
+        end
+      end
+    end
+
+    context "when editing a comment" do
+      let!(:first_comment_by_admin) do
+        create(:work_package_journal, user: admin, notes: "First comment by admin", journable: work_package, version: 2)
+      end
+
+      context "when the update call raises an unknown server error" do
+        before do
+          allow_any_instance_of(WorkPackages::ActivitiesTabController) # rubocop:disable RSpec/AnyInstance
+            .to receive(:update_journal_service_call)
+            .and_raise(StandardError.new("Test error"))
+        end
+
+        it "shows an error banner" do
+          activity_tab.edit_comment(first_comment_by_admin, text: "First comment by admin edited", save: false)
+
+          page.within_test_selector("op-work-package-journal-form-element") do
+            page.find_test_selector("op-submit-work-package-journal-form").click
+          end
+
+          expect_flash(message: "Test error", type: :error)
+        end
+      end
+
+      context "when the update call fails with a validation error" do
+        before do
+          allow_any_instance_of(Journals::UpdateService) # rubocop:disable RSpec/AnyInstance
+            .to receive(:call)
+            .and_return(
+              ServiceResult.failure(errors: ActiveModel::Errors.new(Journal.new).tap do |e|
+                e.add(:notes, "Validation error")
+              end)
+            )
+        end
+
+        it "shows a validation error banner" do
+          activity_tab.edit_comment(first_comment_by_admin, text: "First comment by admin edited", save: false)
+
+          page.within_test_selector("op-work-package-journal-form-element") do
+            page.find_test_selector("op-submit-work-package-journal-form").click
+          end
+
+          expect_flash(message: "Validation error", type: :error)
+        end
+      end
+    end
+  end
 end

spec/support/components/work_packages/activities.rb

@@ -175,18 +175,20 @@ def add_comment(text: nil, save: true)
         end
       end
 
-      def edit_comment(journal, text: nil)
+      def edit_comment(journal, text: nil, save: true)
         within_journal_entry(journal) do
           page.find_test_selector("op-wp-journal-#{journal.id}-action-menu").click
           page.find_test_selector("op-wp-journal-#{journal.id}-edit").click
 
           page.within_test_selector("op-work-package-journal-form-element") do
             FormFields::Primerized::EditorFormField.new("notes", selector: "#work-package-journal-form-element").set_value(text)
-            page.find_test_selector("op-submit-work-package-journal-form").click
+            page.find_test_selector("op-submit-work-package-journal-form").click if save
           end
 
-          # wait for the comment to be loaded
-          wait_for { page }.to have_test_selector("op-journal-notes-body", text:)
+          if save
+            # wait for the comment to be loaded
+            wait_for { page }.to have_test_selector("op-journal-notes-body", text:)
+          end
         end
       end