diff --git a/app/controllers/work_packages/activities_tab_controller.rb b/app/controllers/work_packages/activities_tab_controller.rb index cfe19ecc31c..93ac7ba6cde 100644 --- a/app/controllers/work_packages/activities_tab_controller.rb +++ b/app/controllers/work_packages/activities_tab_controller.rb @@ -106,28 +106,33 @@ class WorkPackages::ActivitiesTabController < ApplicationController 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 @@ class WorkPackages::ActivitiesTabController < ApplicationController # 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 @@ class WorkPackages::ActivitiesTabController < ApplicationController 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 @@ class WorkPackages::ActivitiesTabController < ApplicationController ### 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 diff --git a/frontend/src/app/core/turbo/turbo-requests.service.ts b/frontend/src/app/core/turbo/turbo-requests.service.ts index da2ecacc074..fdef9024175 100644 --- a/frontend/src/app/core/turbo/turbo-requests.service.ts +++ b/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 { diff --git a/frontend/src/stimulus/controllers/dynamic/work-packages/activities-tab/index.controller.ts b/frontend/src/stimulus/controllers/dynamic/work-packages/activities-tab/index.controller.ts index cf2cc74fa9b..ef02a67e404 100644 --- a/frontend/src/stimulus/controllers/dynamic/work-packages/activities-tab/index.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/work-packages/activities-tab/index.controller.ts @@ -398,7 +398,7 @@ export default class IndexController extends Controller { } private getScrollableContainer():HTMLElement | null { - if (this.isWithinNotificationCenter()) { + if (this.isWithinNotificationCenter() || this.isWithinSplitScreen()) { // valid for both mobile and desktop return document.querySelector('.work-package-details-tab') as HTMLElement; } @@ -419,6 +419,10 @@ export default class IndexController extends Controller { return window.location.pathname.includes(this.notificationCenterPathNameValue); } + private isWithinSplitScreen():boolean { + return window.location.pathname.includes('work_packages/details'); + } + private addEventListenersToCkEditorInstance() { this.onSubmitBound = () => { void this.onSubmit(); }; this.adjustMarginBound = () => { void this.adjustJournalContainerMargin(); }; @@ -633,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) { @@ -643,8 +647,8 @@ export default class IndexController extends Controller { if (!this.journalsContainerTarget) return; this.clearEditor(); - this.handleEditorVisibility(); - this.adjustJournalsContainer(); + this.hideEditor(); + this.resetJournalsContainerMargins(); setTimeout(() => { if (this.isMobile() && !this.isWithinNotificationCenter()) { @@ -663,19 +667,11 @@ export default class IndexController extends Controller { this.saveInProgress = false; } - private handleEditorVisibility():void { - if (this.isMobile()) { - this.hideEditorIfEmpty(); - } else { - this.focusEditor(); - } - } - - private adjustJournalsContainer():void { + private resetJournalsContainerMargins():void { if (!this.journalsContainerTarget) return; this.journalsContainerTarget.style.marginBottom = ''; - this.journalsContainerTarget.classList.add('work-packages-activities-tab-index-component--journals-container_with-input-compensation'); + this.journalsContainerTarget.classList.add('work-packages-activities-tab-index-component--journals-container_with-initial-input-compensation'); } private setLastServerTimestampViaHeaders(headers:Headers) { diff --git a/spec/features/activities/work_package/activities_spec.rb b/spec/features/activities/work_package/activities_spec.rb index 5428863f8fd..9bf18426720 100644 --- a/spec/features/activities/work_package/activities_spec.rb +++ b/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 @@ RSpec.describe "Work package activity", :js, :with_cuprite, with_flag: { primeri 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 diff --git a/spec/support/components/work_packages/activities.rb b/spec/support/components/work_packages/activities.rb index 822b458aa08..7b92e6600b6 100644 --- a/spec/support/components/work_packages/activities.rb +++ b/spec/support/components/work_packages/activities.rb @@ -177,18 +177,20 @@ module Components wait_for_network_idle 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