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

Empty values are not getting saved #11373

Closed
vengi83644 opened this issue Mar 14, 2022 · 19 comments
Closed

Empty values are not getting saved #11373

vengi83644 opened this issue Mar 14, 2022 · 19 comments

Comments

@vengi83644
Copy link
Contributor

I know this is weird. But this happens in the Orchard 1.2.2 version.

Repro,

  1. Create a content type with field - a simple textbox and set as not required.
  2. Create content for this new content type filling some text in the textbox. Publish it.
  3. Now, edit the content and remove all the text from the textbox and publish it.
  4. If you open the content now, the text is not removed.

Expected is that the textbox should be empty.

The same goes for checkboxes when there is only one checkbox and unchecking it.

Thanks,
R. Venkatesan

@hishamco
Copy link
Member

Is this happen in the latest bits too? If Yes it's a bug

@vengi83644
Copy link
Contributor Author

In 1.3.0? I haven't tested it. Let me upgrade and try.

Are there any breaking changes other than .net core support? I am already using .net 6.

@hishamco
Copy link
Member

Are there any breaking changes other than .net core support?

AFAIK, there's no breaking in this release except changing the TFM

@sebastienros sebastienros added this to the backlog milestone Mar 17, 2022
@vengi83644
Copy link
Contributor Author

vengi83644 commented Mar 20, 2022

This happens in 1.3.0 too.

I tested this in the text field, link field, and checkboxes.

Repro,

  1. Setup Orchard in Decoupled CMS mode.
  2. Create a content type with a text field.
  3. Create content for this new content type by adding any text in that field and publish it.
  4. Now, edit the content and remove the contents in the text field and publish it.
  5. Again, go to edit page and you will see the contents that you entered initially and not removed in the Step 4

@Skrypt
Copy link
Contributor

Skrypt commented Mar 21, 2022

Which browser are you using when getting this issue? I just tried with the latest preview Nuget packages and I can't repro this issue. We need more details to be able to repro because this doesn't seem to be reproducible by using SQL Server and Google Chrome as the browser.

When you say Decoupled; I assume that you are not using your own custom controller with views to edit your content items but using the actual Orchard Core admin interface to make your changes. Then, here Decoupled doesn't make a difference.

Though, if you are using a custom controller that caches the data then you could potentially have a caching invalidation issue. This looks more like a cache issue than a browser issue to me. Unless your data never gets updated when you submit changes from the Admin UI ... that would be never seen before.

@vengi83644
Copy link
Contributor Author

I tried debugging and found the problem is in the DisplayDriver of each fields

Here is an example that's easy for me to explain.

Take this BooleanFieldDisplayDriver,

The problem I faced in boolean fields is we were able to set that to true and save it. But when we want to uncheck it and save, it won't change and remain true always.

So, I created a Module and copied the "BooleanFieldDisplayDriver" from OC code and added it to my module to debug it.

model.Value is getting set with the latest value(in my case its "false") and the model.Field is still having true and this is why its not updating in the database.

image

I tried setting the model.Field.Content.Value to the latest value and its working - I checked in the database.

This is the same case for Link field, Text field and other fields as well.

Thanks,
R. Venkatesan

@vengi83644
Copy link
Contributor Author

This is what worked for me. I am using version 1.3 and I am going to use this for now.

Added new module and using my own display drivers copied from version 1.3
image

Text Field
image

Bool Field
image

Link Field
image

@vengi83644
Copy link
Contributor Author

@Skrypt @sebastienros

Please let me know your views on this.

@Skrypt
Copy link
Contributor

Skrypt commented May 24, 2022

This is not supposed to be required. You are simply affecting a value manually which gets affected when you do model.Field = field. If this doesn't work then there is an issue related to something that has been changed in your custom code. I never experienced this issue and it seems like a strange fix.

@Skrypt
Copy link
Contributor

Skrypt commented May 24, 2022

image

Here is the proof.

image

@vengi83644
Copy link
Contributor Author

Hey @Skrypt , Thanks for the quick response.

I tried with the sample App that is in Orchard Source. I didn't get this issue in the sample. I agree this is not in Orchard source.

But when I import the content definition from my own app to the sample app, I get the issue. I don't do any other changes to the sample app.

Would my own content types and content parts trigger this issue?

Thanks,
R. Venkatesan

@Skrypt
Copy link
Contributor

Skrypt commented May 24, 2022

Yes, there may be an issue with your Content Definitions. Try to compare them with a Content Type exported from source code solution. If there are differences then let us know. Normally, it should have been migrated if the issue is that you upgraded from a previous version.

@vengi83644
Copy link
Contributor Author

@Skrypt
Thanks. I figured out the problem.

I am using the below JsonSerializerSettings as default in my app.

JsonConvert.DefaultSettings = () =>
            {
                var settings = new JsonSerializerSettings()
                {
                    NullValueHandling = NullValueHandling.Ignore,
                    DefaultValueHandling = DefaultValueHandling.Ignore
                };

                return settings;
            };

This conflicts with the settings(JsonMergeSettings I hope) in OC.

Please let me know your suggestions to overcome this.

Thanks,
R. Venkatesan

@Skrypt
Copy link
Contributor

Skrypt commented May 30, 2022

You could try to create a scope that uses these settings only when requested for the parts that you require to use them. Else, changing the defaults of Orchard Core is probably not a good idea.

@vengi83644
Copy link
Contributor Author

OK, but I am using this setting in my MVC app as a default setting for all my JSON serialization.

I don't want OC to pick this setting as I hope it has its own setting in it.

Can you please explain how do we scope my setting to my app alone?

@vengi83644
Copy link
Contributor Author

@Skrypt Any inputs?

@jtkech
Copy link
Member

jtkech commented May 30, 2022

Hmm, we use a lot Newtonsoft and I don't think we override any System.Text.Json settings at the tenant/modules level, meaning that we use the inherited default settings. So you may need to do revert your custom settings at the tenant level.

We have Configure() and ConfigureServices() helpers to do it from the app but executed at the tenant/modules level.

.AddOrchardCms()
.ConfigureServices(tenantServices =>
{
    // Some stuff.
}

@vengi83644
Copy link
Contributor Author

Thanks @jtkech. Let me try and update here.

@vengi83644
Copy link
Contributor Author

Thanks @jtkech @Skrypt for helping me in identifying the root cause,

I have fixed the issue. Here is what worked for me,

I am setting this in my .Net core app

JsonConvert.DefaultSettings = () =>
            {
                var settings = new JsonSerializerSettings()
                {
                    NullValueHandling = NullValueHandling.Ignore,
                    DefaultValueHandling = DefaultValueHandling.Ignore
                };

                return settings;
            };

and I am setting this in the OC tenant settings,
I have set the null and default handling to "Include" to match with JsonMergeSettings in OC

builder.Services.AddOrchardCms()
                .AddMvc()
                .AddDatabaseShellsConfiguration()
                .ConfigureServices((services) =>
                {
                    JsonConvert.DefaultSettings = () =>
                    {
                        var settings = new JsonSerializerSettings()
                        {
                            NullValueHandling = NullValueHandling.Include,
                            DefaultValueHandling = DefaultValueHandling.Include
                        };

                        return settings;
                    };

                    services.ConfigureHtmlSanitizer((sanitizer) =>
                    {
                        sanitizer.AllowedSchemes.Add("mailto");
                    });
                });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants