From b1dc57324e57ebdb62e2b0c968281c6cc7dadf96 Mon Sep 17 00:00:00 2001 From: PointStar Date: Sun, 24 May 2026 19:21:28 +0300 Subject: [PATCH] fix(save-attachments): authoritative disk-verify + hide extension in rename UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs reported after v1.1.2: 1. False-negative "נכשלו" toast even though the file was on disk. Root cause from the log: Polly's 15s timeout on /NetworkStorage/action/upload fires while the server is still writing larger PDFs, so the client sees TimeoutRejectedException → Outcome.Failed, while the server quietly finishes the write. The outcome is now decided by listing the target folder, not by the upload response. If the upload exception fires, the listing is retried up to 4 times with 2s delays to let the server finish. Saved if the file appears, Failed only if it never does. 2. The filename TextBox in the attachments list exposed the extension (.pdf / .docx). Users have to delete it manually when renaming and risk dropping it. AttachmentRowViewModel now splits FileName into BaseName (editable) and Extension (frozen at construction). The XAML binds the TextBox to BaseName and shows the Extension as a muted adjacent chip. The on-disk filename = BaseName + Extension always. Bump to 1.1.3.0 for the patch release. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Services/AttachmentSaveService.cs | 172 ++++++++++++------ .../Dialogs/SaveAttachmentsDialog.xaml | 10 +- .../ViewModels/AttachmentRowViewModel.cs | 30 ++- src/OutlookAddin/OutlookAddin.csproj | 2 +- src/OutlookAddin/Properties/AssemblyInfo.cs | 4 +- 5 files changed, 150 insertions(+), 68 deletions(-) diff --git a/src/OutlookAddin.Core/Services/AttachmentSaveService.cs b/src/OutlookAddin.Core/Services/AttachmentSaveService.cs index 9af7f5a..cb04925 100644 --- a/src/OutlookAddin.Core/Services/AttachmentSaveService.cs +++ b/src/OutlookAddin.Core/Services/AttachmentSaveService.cs @@ -46,35 +46,17 @@ namespace MarcusLaw.OutlookAddin.Core.Services continue; } + string? attachmentId = null; + Exception? uploadException = null; + + // Step 1: create the EspoCRM Attachment (binary stored in + // data/upload/; not yet on the network share). Auth + // failures here are terminal — skip remaining items. try { - // Step 1: create the EspoCRM Attachment (binary stored - // in data/upload/; not yet on the network share). var uploaded = await _client.UploadAttachmentAsync( att.Name, att.ContentType, att.ContentBase64, cancellationToken).ConfigureAwait(false); - - // Step 2: ask Marcus-Law's NetworkStorageIntegration - // to place the uploaded blob under . - // The integration appends the filename to the path. - await _client.UploadAttachmentToNetworkStorageAsync( - uploaded.Id, trimmedFolder, cancellationToken).ConfigureAwait(false); - - // Step 3: verify the file actually landed on the share — - // the server can return 200 yet leave nothing on disk - // when a downstream WebDAV proxy silently fails. Read - // the folder back and look for our filename. - await VerifyArrivedAsync(att.Name, trimmedFolder, cancellationToken).ConfigureAwait(false); - - _logger.Information( - "Saved attachment {Name} → {Path} (attachmentId={AttachmentId})", - att.Name, trimmedFolder, uploaded.Id); - - summary.Items.Add(new AttachmentSaveItemResult - { - FileName = att.Name, - Outcome = AttachmentSaveOutcome.Saved, - AttachmentId = uploaded.Id - }); + attachmentId = uploaded.Id; } catch (EspoCrmAuthorizationException ex) { @@ -86,20 +68,85 @@ namespace MarcusLaw.OutlookAddin.Core.Services Outcome = AttachmentSaveOutcome.AuthRequired, ErrorMessage = ex.Message }); + continue; } - catch (OperationCanceledException) - { - throw; - } + catch (OperationCanceledException) { throw; } catch (Exception ex) { - _logger.Warning(ex, "Attachment save failed for {Name}", att.Name); + // No attachment ID means we have nothing on the share — + // a hard fail. + _logger.Warning(ex, "Attachment upload (Step 1) failed for {Name}", att.Name); summary.Items.Add(new AttachmentSaveItemResult { FileName = att.Name, Outcome = AttachmentSaveOutcome.Failed, ErrorMessage = ex.Message }); + continue; + } + + // Step 2: ask the NetworkStorageIntegration to place the + // uploaded blob under . The 15s Polly + // timeout sometimes fires while the server is still + // writing the file — capture the exception but don't + // decide the outcome yet. + try + { + await _client.UploadAttachmentToNetworkStorageAsync( + attachmentId, trimmedFolder, cancellationToken).ConfigureAwait(false); + } + catch (EspoCrmAuthorizationException ex) + { + _logger.Warning(ex, "Attachment save aborted on Step 2: EspoCRM rejected credentials"); + summary.AuthRequired = true; + summary.Items.Add(new AttachmentSaveItemResult + { + FileName = att.Name, + Outcome = AttachmentSaveOutcome.AuthRequired, + ErrorMessage = ex.Message + }); + continue; + } + catch (OperationCanceledException) { throw; } + catch (Exception ex) + { + uploadException = ex; + _logger.Warning(ex, + "Attachment NetworkStorage upload (Step 2) reported failure for {Name}; will verify disk before declaring failure", + att.Name); + } + + // Step 3: verify on disk. Authoritative — if the file is + // there, the save succeeded regardless of what Step 2 + // reported (Polly timeouts during large writes are common). + var verified = await VerifyArrivedWithRetryAsync( + att.Name, trimmedFolder, uploadException != null, cancellationToken).ConfigureAwait(false); + + if (verified) + { + _logger.Information( + "Saved attachment {Name} → {Path} (attachmentId={AttachmentId}, uploadException={HadException})", + att.Name, trimmedFolder, attachmentId, uploadException != null); + summary.Items.Add(new AttachmentSaveItemResult + { + FileName = att.Name, + Outcome = AttachmentSaveOutcome.Saved, + AttachmentId = attachmentId + }); + } + else + { + var message = uploadException?.Message + ?? "Server reported success but the file is not on the share."; + _logger.Warning( + "Attachment save Failed for {Name}: not present after verify (uploadException={Msg})", + att.Name, message); + summary.Items.Add(new AttachmentSaveItemResult + { + FileName = att.Name, + Outcome = AttachmentSaveOutcome.Failed, + ErrorMessage = message + }); } } @@ -107,51 +154,60 @@ namespace MarcusLaw.OutlookAddin.Core.Services } /// - /// Post-upload sanity check: list the folder we just wrote into - /// and confirm a file with the same (possibly sanitized) name - /// shows up. Diagnostic only — does not change the outcome. If - /// the file isn't there, log a WARNING with everything that IS - /// in the folder so we can spot the actual server-side path. + /// Authoritative post-upload check. Returns true if the file is on + /// disk under . When the upload call + /// raised (typically Polly's 15s timeout on a still-writing server), + /// we retry the listing a few times so the server gets a chance + /// to finish. /// - private async Task VerifyArrivedAsync(string expectedName, string folderPath, CancellationToken ct) + private async Task VerifyArrivedWithRetryAsync(string expectedName, string folderPath, bool afterFailure, CancellationToken ct) + { + // When the upload appeared to succeed, one check is plenty — the + // file is already there. When it appeared to fail, the server + // may still be writing; give it a few extra seconds. + var attempts = afterFailure ? 4 : 1; + var delay = TimeSpan.FromSeconds(2); + + for (int i = 0; i < attempts; i++) + { + ct.ThrowIfCancellationRequested(); + if (i > 0) await Task.Delay(delay, ct).ConfigureAwait(false); + + if (await IsPresentAsync(expectedName, folderPath, ct).ConfigureAwait(false)) + { + if (i > 0) + { + _logger.Information( + "Post-upload verify: '{Expected}' appeared under '{Path}' on attempt {Attempt}/{Total} despite upload exception", + expectedName, folderPath, i + 1, attempts); + } + return true; + } + } + return false; + } + + private async Task IsPresentAsync(string expectedName, string folderPath, CancellationToken ct) { try { var items = await _client.ListNetworkStorageFolderAsync(folderPath, ct).ConfigureAwait(false); var trimmedExpected = expectedName.Trim().TrimStart('-', ' ').TrimEnd('-', ' '); - bool found = false; foreach (var it in items) { if (it.IsFolder) continue; if (string.Equals(it.Name, expectedName, StringComparison.Ordinal) || string.Equals(it.Name, trimmedExpected, StringComparison.Ordinal)) { - found = true; - break; + return true; } } - if (!found) - { - var names = new List(); - foreach (var it in items) - { - if (it.IsFolder) continue; - names.Add(it.Name); - } - _logger.Warning( - "Post-upload verify: expected '{Expected}' under '{Path}' but it's NOT in the listing. " + - "Server reported the upload as successful but the file is not on the share. " + - "Files currently in that folder: [{Files}]", - expectedName, folderPath, string.Join(", ", names)); - } - else - { - _logger.Information("Post-upload verify: '{Expected}' present under '{Path}'", expectedName, folderPath); - } + return false; } catch (Exception ex) { - _logger.Warning(ex, "Post-upload verify failed for '{Expected}' under '{Path}'", expectedName, folderPath); + _logger.Warning(ex, "Post-upload listing under '{Path}' failed", folderPath); + return false; } } } diff --git a/src/OutlookAddin.UI/Dialogs/SaveAttachmentsDialog.xaml b/src/OutlookAddin.UI/Dialogs/SaveAttachmentsDialog.xaml index acdfb83..3f8b721 100644 --- a/src/OutlookAddin.UI/Dialogs/SaveAttachmentsDialog.xaml +++ b/src/OutlookAddin.UI/Dialogs/SaveAttachmentsDialog.xaml @@ -184,14 +184,20 @@ FontSize="11" VerticalAlignment="Center" Margin="8,0,0,0" /> - + + ToolTip="לחץ כדי לערוך את שם הקובץ (הסיומת נשמרת אוטומטית)" /> diff --git a/src/OutlookAddin.UI/ViewModels/AttachmentRowViewModel.cs b/src/OutlookAddin.UI/ViewModels/AttachmentRowViewModel.cs index 1123a45..5ab33f8 100644 --- a/src/OutlookAddin.UI/ViewModels/AttachmentRowViewModel.cs +++ b/src/OutlookAddin.UI/ViewModels/AttachmentRowViewModel.cs @@ -1,3 +1,4 @@ +using System.IO; using CommunityToolkit.Mvvm.ComponentModel; using MarcusLaw.OutlookAddin.Core.Models; @@ -10,23 +11,42 @@ namespace MarcusLaw.OutlookAddin.UI.ViewModels [ObservableProperty] private bool isSelected = true; - public string FileName + /// + /// Frozen at construction from the original filename. The user + /// edits only the base name; the extension always tags along. + /// + public string Extension { get; } + + /// + /// Editable base name without the extension. Setting this rewrites + /// as <basename><Extension>. + /// + public string BaseName { - get => Data.Name; + get => Path.GetFileNameWithoutExtension(Data.Name) ?? string.Empty; set { - var newName = value ?? string.Empty; - if (Data.Name == newName) return; - Data.Name = newName; + var newBase = (value ?? string.Empty).Trim(); + var newFull = newBase + Extension; + if (Data.Name == newFull) return; + Data.Name = newFull; OnPropertyChanged(); + OnPropertyChanged(nameof(FileName)); } } + /// + /// Full filename including extension — what actually goes to the + /// server. Read-only mirror of . + /// + public string FileName => Data.Name; + public string SizeDisplay { get; } public AttachmentRowViewModel(EspoAttachment data) { Data = data; + Extension = Path.GetExtension(data.Name) ?? string.Empty; SizeDisplay = ComputeSize(data.ContentBase64); } diff --git a/src/OutlookAddin/OutlookAddin.csproj b/src/OutlookAddin/OutlookAddin.csproj index d571eec..11256dd 100644 --- a/src/OutlookAddin/OutlookAddin.csproj +++ b/src/OutlookAddin/OutlookAddin.csproj @@ -36,7 +36,7 @@ C:\Users\Chaim\source\repos\OutlookAddin\Publish\ https://gitea.dev.marcus-law.co.il/espocrm-extensions/OutlookAddin/raw/branch/main/Publish/ en - 1.1.2.0 + 1.1.3.0 false true 7 diff --git a/src/OutlookAddin/Properties/AssemblyInfo.cs b/src/OutlookAddin/Properties/AssemblyInfo.cs index c848049..7ac3510 100644 --- a/src/OutlookAddin/Properties/AssemblyInfo.cs +++ b/src/OutlookAddin/Properties/AssemblyInfo.cs @@ -33,6 +33,6 @@ using System.Security; // You can specify all the values or you can default the Build and Revision Numbers // by using the '*' as shown below: // [assembly: AssemblyVersion("1.0.*")] -[assembly: AssemblyVersion("1.1.2.0")] -[assembly: AssemblyFileVersion("1.1.2.0")] +[assembly: AssemblyVersion("1.1.3.0")] +[assembly: AssemblyFileVersion("1.1.3.0")]