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

Conversation

kkwangsir
Copy link
Contributor

In the past, it was monitored that there were some concurrency errors in the API during content editing. After a long period of comparison, I found that most of it was actually harmless.
Some of it was caused by the request for a transcript. I tried automatic merging, but it turned out to be unsafe, as there were too many factors to consider.
So, I changed it to display the modified fields on the frontend, giving users a prompt, and logging some detailed content on the backend for debugging purposes.

image

@JacobWang-bc JacobWang-bc added the enhancement New feature or request label Jan 22, 2025

// Throw user-friendly message with better formatting
var userMessage = $"Content has been modified by another user. Modified fields: '{changedFields}'. " +
"Please save your changes, refresh page, and reapply your updates.";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's impossible for the user to save their changes before refreshing the page. It will result in the same error.

A better user experience would be to inform the user which fields are different. Then they could choose whether they want to override the values in the database with their own. You just need to be careful here, because many users will just say override. Ideally we would have a way to determine what was changed, rather than what is different. For example if the first save changes fields A and B, and the second save changes fields B and C. Technically we can automatically resolve changes to fields A and C, but need to ask the user what to do with field B because they both edited it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, for the helpful input. I think I may need to discuss some of this with the designer and modify the UI.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally if there is no difference other than the version number, or the status, it should be able to merge and save correctly without returning an error.

@JacobWang-bc JacobWang-bc marked this pull request as draft January 22, 2025 17:35
…dle concurrency conflicts more gracefully. Added methods to retrieve and log changed properties, improving user feedback on modifications made by others.
- Add ContentConflictException to middleware for handling content update conflicts
- Update useToastError hook to display detailed conflict messages with modified fields
- Modify ContentService to use ContentConflictException
@JacobWang-bc JacobWang-bc marked this pull request as ready for review February 6, 2025 22:28
@JacobWang-bc
Copy link
Collaborator

JacobWang-bc commented Feb 6, 2025

image

It's been a while since we've found with this issue. Based on the designer's feedback, let's keep the current toast display approach but make it clearer for the user. If there's a content conflict, show the modified fields. For the ContentConflictException error specifically, manually close the toast to give users enough time to read and act.

}
catch (DbUpdateConcurrencyException ex)
{
this.Logger.LogInformation("Concurrency conflict detected - ContentId: {ContentId}", entity.Id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this as error log too?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, it is a good idea.

Copy link
Collaborator

@nehalaggarwal-bcgov nehalaggarwal-bcgov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. Good work on adding comprehensive Exception handling code.

@JacobWang-bc JacobWang-bc merged commit b1b4dc3 into bcgov:dev Feb 14, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants