Display Drivers validation issue

[Skrypt]

Should always return updated model for displaying proper data even if the model has failed to validate.

[Skrypt]

Here normally we call Edit(ViewModel) and this is why we need to return and update the model instead which is because of how the DisplayDriver works. I looked at it to see if we could always pass a TModel and TViewModel but that seems a lot of work and breaking change. Also, somehow it is possible to use a viewModel still. It just feels hacky to always update the model instead of updating the viewModel. But the model will never be persisted if there is a model state validation error.

[deanmarcussen]

noting that this issue is common across all display drivers in OC, as the design pattern has only ever been to update the viewmodel if validation passed.
it's more common in workflows because more data attributes are used there, than on other drivers. (and the data attributes cause TryValidate to return false if validation fails.

This means that when two properties are required and only one is present, the value for the other gets lost.

We updated all our drivers not to behave like OC, which resolved the issue

[Skrypt]

@Piedone See this one who probably will need a lot of manual testing.

Basically, the issue is that right now we are using an inherited ActivityDisplayDriver to override the UpdateAsync method because our drivers fails to update their models because of these If conditions everywhere. This mainly cause issue with ViewModels that have fields with ­a [Required] attribute for example.

I tried looking at the base DisplayDriver generics and how they work but none of them makes a distinction between the ViewModel and the Model. There is one using a TConcrete which seems to be why we can't specify a TViewModel. Basically, we need to always update the model even when there is a validation exception which is kind of not something advertised to do normally.

The proper workflow is to pass the viewModel and to update the model only when ModelState.IsValid normally. But here, the way the DisplayDriver is designed we can't do this because it only passes the base model all the time.

[Piedone]

I'm not sure I understand. Can you upload a screen recording with before-after?

I see that now the DB models, not just the view models are updated regardless of the model's validity. While we won't usually save invalid values due to the transaction being canceled later, I don't really like that we write to the DB models. However, I understand this should retain some values after validation errors; but I don't see it happening.

E.g. SearchPartViewModel has two fields, one required. I'd expect the below issue with half of the form disappearing to not happen, but it does exactly in the same way as with main:

2024-03-14_00h10_49.mp4

Shouldn't this be fixed?

Additionally, I don't think this is a good approach to fix the issue. Apparently, there's a shortcoming on OC's API. Instead of working around it with, we should fix the API. E.g., we could have an Edit() override in drivers that optionally takes a ViewModel too.

[Skrypt]

Yes, but I've not found yet a way to do this without breaking the actual entire driver system. I've taken a quick look, I need more time to think about it. Basically something with public abstract class DisplayDriver

And I think the issue is because of:

public abstract class DisplayDriver<TModel, TConcrete>

[Skrypt]

@Piedone Here we go. There is also sound on that one as an explanation.

2024-03-13.22-50-29.mp4

[Piedone]

Thank you!

In the case of ActivityDisplayDriver it seems simpler, since the implementations like EmailTaskDisplayDriver don't need to change, just ActivityDisplayDriver.UpdateAsync() should pass viewModel to Edit. Now I know this is not so simple (if we want to do it properly), but it seems to me that the call chain down to DisplayDriverBase and below can also be adjusted to have additional overrides that take a model object (ultimately ShapeFactoryExtensions.CreateShape() (you'll need to use _proxyGenerator.CreateClassProxyWithTarget() for these though).

And then something similar for content drivers.

[sebastienros]

Here is a repro that explain why it works today:

• Configuration -> Admin Menu -> add a new Admin Link Node
• Add values in Link Text and Link Urls which are required
• Save
• Edit the node, remove the value from Link Text, validation works, values are as typed
• Add a permission, Save
• Notice permission is not maintained

This is because Link Text is bound with <label asp-for which seems to be using the form request data instead of the Model. While the permissions editor is rendered using the Model (a view model) directly, and the Model is the one from the database, as returned by the Edit method like a new Edition.

I will think about the options:

• Returning the view model from Update (but too much breaking if not impossible)
• Doing what Jasmin did, and Dean apparently

Not all the drivers would need to be updated, they are not broken, but we should still do it if we find a good pattern.

[Skrypt]

I would really prefer that we find a solution with first option.

Returning the view model from Update (but too much breaking if not impossible)

Main reason is about the fact that it makes the coding experience quite different than when using a Controller with a Edit and Post actions that simply return the ViewModel.

This cause a lot of need to create extra code in the Edit method when you have ViewModel elements that are not updating the database as an example.

ViewModel.HasData ...

Edit (){
if (service.HasData)
{
ViewModel.HasData
}
}

then I need to do this binding back in the Model with a no binding attribute to update the model to send it back to the edit method. That's a lot of extra code for doing something that should work with the design pattern of using a ViewModel.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Skrypt]

Note to myself. We can do:

if (context is UpdateEditorContext) { //set values modified by user with passed model } on EditAsync to distinguish from a call that comes back from a validation error for example.
This way, if the form element has a default value and a user wants to update a ViewModel value we can make a distinction between both.

Though, I need to test it to see if it would fix the issue with the permissions editor as an example. Right now, I don't think it would.

[Skrypt]

Actually just tested this on the Admin Menu editor. And the changes I made in this PR fixes the issue about Permissions retained when a validation issue happens.

Before:

After:

This is happening because I removed that if and we pass back that model/viewModel value.

OrchardCore/src/OrchardCore.Modules/OrchardCore.AdminMenu/AdminNodes/LinkAdminNodeDriver.cs

Lines 56 to 74 in 74d923c

	public override async Task<IDisplayResult> UpdateAsync(LinkAdminNode treeNode, IUpdateModel updater)
	{
	var model = new LinkAdminNodeViewModel();
	if (await updater.TryUpdateModelAsync(model, Prefix, x => x.LinkUrl, x => x.LinkText, x => x.IconClass, x => x.SelectedPermissionNames))
	{
	treeNode.LinkText = model.LinkText;
	treeNode.LinkUrl = model.LinkUrl;
	treeNode.IconClass = model.IconClass;
	var selectedPermissions = (model.SelectedPermissionNames == null ? [] : model.SelectedPermissionNames.Split(',', StringSplitOptions.RemoveEmptyEntries));
	var permissions = await _adminMenuPermissionService.GetPermissionsAsync();
	treeNode.PermissionNames = permissions
	.Where(p => selectedPermissions.Contains(p.Name))
	.Select(p => p.Name).ToArray();
	}
	return Edit(treeNode);
	}

[Skrypt]

@sebastienros Is there a reason why this SectionDisplayDriver uses BuildEditorContext on the UpdateAsync instead of UpdateEditorContext here. It is pretty useful to make a distinction. I tried this change and it still works where I tested it.

[Skrypt]

So basically the idea is that. We pass back the model from the UpdateAsync method and then in the EditAsync method we check for context is UpdateEditorContext to see if it comes back from the updateAsync method. Then we can apply changes to the model accordingly in the EditAsync.

One issue we have is that we have something like

EditAsync(Something model, BuildEditorContext context) {
   var service = await dbCall();
   if (service.Entries.Any())
   {
      model.HasData = true;
   }
   
   // do some other stuff

   if (context.IsNew && model.HasData && context is not UpdateEditorContext)
  {
     model.IsEnabled = true
  }
}

context.IsNew can be true on a validation error when it is posted back. That's the issue and why we need to have a UpdateEditorContext check.

So there is a workaround to validation issues if we at least pass the proper UpdateEditorContext with these UpdateAsync methods.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Skrypt]

@sebastienros Any progress on this on your side? I can't commit more time on this one tbh. Have other PR's to work on. Hoped that you would give some feedback on some progress.

[Skrypt]

Found another issue where I'm using .ToSafeName() in the UpdateAsync method and if on the .cshtml Razor view I'm using asp-for ; the value is never updated. I also need to throw a custom validation exception instead of just naturally let the tryUpdateModel method update the ModelState. That's just not feeling right.

[sebastienros]

Should we update the field itself (which is bound to the content item, which is stored in the db eventually) or should we only update View Model properties such that there is no chance we save the results if that fails. And only update the field if the TryUpdateModeAsync succeeds, like with the previous code.

This is a general question, not just specific to this driver.

[Piedone]

Always just the viewmodel.

[Skrypt]

Updating the Model instead of the ViewModel just leads into needing to do patching code here and there which is awkward.

[sebastienros]

So we agree. And here it's updating the model, the field, instead of the view model which is created at the top of the method. Should we go through all these cases too or trust content manager to not save the content item because there were errors (pretty sure that's how it works).

[Skrypt]

Yes that's how it works.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Piedone]

Wait, wasn't this prematurely merged? Is this really fully solving the issue? Also, this seems like a breaking change, which needs to be documented in the release notes, and since the recommendations for module authors also changed regarding how to handle ViewModel updates, that needs to be added to the release notes too.

[Skrypt]

It was discussed on today's meeting that even if we are adding obsolete methods it will break the SectionDisplayDriver usage. It is not solving the entire issue but I will dig further down the issue by myself on my spare time.
The only thing needed to be added in the Readme is to say that we broke the SectionDisplayDriver by using the UpdateEditorContext.

Of course the issue with the asp-for still stands but at least this PR fixes the values returned to the Edit method so that when a validation error happens that the user entered values be persisted in the UI. The proper fix will require more thinking.

[Piedone]

I see, thanks for explaining. Would it be good to have an issue for this for visibility, and to keep brainstorming there?