Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MMI-3063 better UX for concurrency errors #2391

Merged
merged 11 commits into from
Feb 14, 2025
Merged
18 changes: 17 additions & 1 deletion api/net/Middleware/ErrorHandlingMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,16 @@ private async Task HandleExceptionAsync(HttpContext context, Exception ex)
else if (ex is DbUpdateConcurrencyException)
{
code = HttpStatusCode.BadRequest;
message = "Data may have been modified or deleted since item was loaded. Refresh your data and reapply your changes.";
if (ex.InnerException is ContentConflictException)
{
message = ex.InnerException.GetTypeName();
details = ex.InnerException.Message;

}
else
{
message = "Data may have been modified or deleted since item was loaded. Refresh your data and reapply your changes.";
}

_logger.LogError(ex,
"Optimistic concurrency error (user agent: {userAgent})",
Expand Down Expand Up @@ -167,6 +176,13 @@ private async Task HandleExceptionAsync(HttpContext context, Exception ex)

_logger.LogError(ex, "Configuration error. {error}", ex.Message);
}
else if (ex is ContentConflictException contentConflictEx)
{
code = HttpStatusCode.Conflict;
message = contentConflictEx.Message;

_logger.LogError(ex, "Content conflict detected: {error}", ex.Message);
}
else
{
_logger.LogError(ex, "Middleware caught unhandled exception. {error}", ex.GetAllMessages());
Expand Down
69 changes: 60 additions & 9 deletions app/editor/src/store/hooks/app/useToastError.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
import React from 'react';
import { toast } from 'react-toastify';
import { toast, ToastOptions } from 'react-toastify';
import { Col } from 'tno-core';

import { useApp } from './useApp';

// Constants for error codes
const EXCEPTION_TEXT = {
CONTENT_CONFLICT: 'ContentConflictException',
DB_UPDATE_CONCURRENCY: 'DbUpdateConcurrencyException',
};

// Common styles
const styles = {
message: { margin: '0' },
title: { margin: '0', fontWeight: 'bold' },
container: { marginBottom: '8px' },
} as const;

export const useToastError = () => {
const [state, api] = useApp();

const [errors, setErrors] = React.useState(state.errors);

React.useEffect(() => {
Expand All @@ -20,13 +32,52 @@ export const useToastError = () => {

React.useEffect(() => {
errors.forEach((e) => {
toast.error(
<Col>
<p>{e.message}</p>
<p>{e.detail}</p>
</Col>,
{},
);
const toastOptions: ToastOptions = {
position: 'top-right',
};

// content conflict
if (
e.data?.type === EXCEPTION_TEXT.DB_UPDATE_CONCURRENCY &&
e.message === EXCEPTION_TEXT.CONTENT_CONFLICT
) {
toastOptions.autoClose = false;
toastOptions.closeButton = true;
toastOptions.className = 'concurrency-conflict-toast';

// extract modified fields from error message
const fieldsMatch = e.detail?.match(/^FIELDS:(.+)$/);
const modifiedFields = fieldsMatch ? fieldsMatch[1].split(',').join(', ') : '';

toast.error(
<Col className="concurrency-conflict-message">
<div>
<div style={styles.container}>
<p style={styles.title}>
Your changes could not be saved. Updates from another user could not be loaded.
</p>
</div>
{modifiedFields && (
<p style={styles.container}>
Different fields: <span style={styles.title}>{modifiedFields}</span>
</p>
)}
<p style={styles.message}>
Please copy your changes elsewhere, then refresh the page to apply the updates.
</p>
</div>
</Col>,
toastOptions,
);
} else {
toast.error(
<Col>
<p style={styles.message}>{e.message}</p>
<p style={styles.message}>{e.detail}</p>
</Col>,
toastOptions,
);
}
});
}, [errors]);
};
20 changes: 20 additions & 0 deletions libs/net/core/Exceptions/ContentConflictException.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
using System;

namespace TNO.Core.Exceptions
{
/// <summary>
/// Exception thrown when content is modified by another user during update.
/// </summary>
public class ContentConflictException : Exception
{
/// <summary>
/// Creates a new instance of a ContentConflictException object.
/// </summary>
/// <param name="message"></param>
/// <param name="innerException"></param>
public ContentConflictException(string message, Exception? innerException = null)
: base(message, innerException)
{
}
}
}
11 changes: 11 additions & 0 deletions libs/net/core/Extensions/ExceptionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ public static string GetAllMessages(this Exception ex)
return $"{ex.Message} {ex.InnerException?.GetAllMessages()}";
}


/// <summary>
/// Get full exception type name
/// </summary>
/// <param name="ex"></param>
/// <returns></returns>
public static string GetTypeName(this Exception ex)
{
return ex.GetType().Name;
}

/// <summary>
/// Throw an ArgumentNullException if the value is null.
/// </summary>
Expand Down
122 changes: 114 additions & 8 deletions libs/net/dal/Services/ContentService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -288,17 +288,123 @@
}

/// <summary>
/// Update the specified 'entity' in the database.
/// Get the changed properties between current values and database values, display to users .
/// Ignores system fields type fields to focus on business-relevant changes.
/// </summary>
/// <param name="entity"></param>
/// <returns></returns>
/// <exception cref="NoContentException"></exception>
/// <param name="currentValues">Current property values from the entity being saved</param>
/// <param name="databaseValues">Current property values from the database</param>
/// <returns>Dictionary of changed properties with their current and database values</returns>
private Dictionary<string, object> GetChangedProperties(Microsoft.EntityFrameworkCore.ChangeTracking.PropertyValues currentValues,
Microsoft.EntityFrameworkCore.ChangeTracking.PropertyValues? databaseValues)
{
// Fields to ignore: system fields
var ignoreFields = new HashSet<string> {
nameof(Content.Versions),
nameof(Content.UpdatedBy),
nameof(Content.UpdatedOn),
nameof(Content.Version),
nameof(Content.CreatedBy),
nameof(Content.CreatedOn),
nameof(Content.PostedOn),
};

var propertyNames = currentValues.Properties
.Select(p => p.Name)
.Where(name => !ignoreFields.Contains(name)); // Filter out ignored fields

var valueComparison = propertyNames.ToDictionary(
name => name,
name => new
{
Current = currentValues[name],
Database = databaseValues?[name]
}
);

return valueComparison
.Where(kvp => !Equals(kvp.Value.Current, kvp.Value.Database))
.ToDictionary(
kvp => kvp.Key,
kvp => new
{
CurrentValue = kvp.Value.Current?.ToString() ?? "null",
DatabaseValue = kvp.Value.Database?.ToString() ?? "null"
} as object
);
}

/// <summary>
/// Log the detailed changes between current and database values for each changed field.
/// </summary>
/// <param name="changedProperties">Dictionary containing the changed properties and their values</param>
private void LogFieldChanges(Dictionary<string, object> changedProperties)
{
var changes = changedProperties.ToDictionary(
kvp => kvp.Key,
kvp =>
{
var values = (dynamic)kvp.Value;
return new
{
Previous = values.DatabaseValue,
Current = values.CurrentValue
};
}
);

this.Logger.LogInformation("Field changes detected:\n" +
string.Join("\n", changes.Select(c =>
$"Field: {c.Key}\n" +
$" - Previous: {c.Value.Previous}\n" +
$" - Current: {c.Value.Current}"
))
);
}

/// <summary>
/// Update and save the content entity. Handles concurrency conflicts by providing detailed information about changes.
/// </summary>
/// <param name="entity">The content entity to update</param>
/// <returns>The updated content entity</returns>
/// <exception cref="NoContentException">Thrown when the entity does not exist</exception>
/// <exception cref="DbUpdateConcurrencyException">Thrown when a concurrency conflict is detected</exception>
public override Content UpdateAndSave(Content entity)
{
var original = FindById(entity.Id) ?? throw new NoContentException("Entity does not exist");
this.Context.UpdateContext(original, entity);
if (entity.GuaranteeUid() && original.Uid != entity.Uid) original.Uid = entity.Uid;
return base.UpdateAndSave(original);
try
{
var original = FindById(entity.Id) ?? throw new NoContentException("Entity does not exist");
this.Context.UpdateContext(original, entity);
if (entity.GuaranteeUid() && original.Uid != entity.Uid) original.Uid = entity.Uid;
return base.UpdateAndSave(original);
}
catch (DbUpdateConcurrencyException ex)
{
this.Logger.LogError("Concurrency conflict detected - ContentId: {ContentId}", entity.Id);

var entry = ex.Entries.Single();
var currentValues = entry.CurrentValues;
var databaseValues = entry.GetDatabaseValues();

if (databaseValues == null)
{
var errorMessage = "Unable to retrieve latest content.";
this.Logger.LogError(ex, errorMessage);
throw new NoContentException(errorMessage, ex);
}

// Get changed properties and log details
var changedProperties = GetChangedProperties(currentValues, databaseValues);
LogFieldChanges(changedProperties);

if (changedProperties.Keys.Any())
{
var changedFields = string.Join(",", changedProperties.Keys);
var contentConflictEx = new ContentConflictException($"FIELDS:{changedFields}");
throw new DbUpdateConcurrencyException(contentConflictEx.Message, contentConflictEx);
}

throw ex;

Check warning on line 406 in libs/net/dal/Services/ContentService.cs

View workflow job for this annotation

GitHub Actions / build-net-libs

Re-throwing caught exception changes stack information (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2200)

Check warning on line 406 in libs/net/dal/Services/ContentService.cs

View workflow job for this annotation

GitHub Actions / build-net-libs

Re-throwing caught exception changes stack information (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2200)
}
}

/// <summary>
Expand Down
Loading