[CLOSED] [#611] Can DirectResult support success false without requiring ErrorMessage to be set?

[anup]

Hi,

Looking at DirectResult I see this:

        public override void ExecuteResult(ControllerContext context)
        {
            DirectResponse response = new DirectResponse();

            response.Result = this.Result;
            response.IsUpload = this.IsUpload;

            response.Success = ResourceManager.AjaxSuccess;
            response.ErrorMessage = ResourceManager.AjaxErrorMessage;

            if (!string.IsNullOrEmpty(this.ErrorMessage))
            {
                response.Success = false;
                response.ErrorMessage = this.ErrorMessage;
            }
            else
            {                
                if (!string.IsNullOrEmpty(this.Script))
                {
                    response.Script = this.Script;
                }

                if (this.ExtraParamsResponse.Count > 0)
                {
                    response.ExtraParamsResponse = this.ExtraParamsResponse.ToJson();
                }
            }
            
            response.Return();
        }

For the response.Success to be false, I must set the ErrorMessage.

However, I have times where I set the error message on the client side (JavaScript) as I format some complicated HTML which I cannot easily do in code behind, because of the various bits of data and state that the JavaScript has.

Would it be possible to extend DirectResult in the following way:
1. Have a property, Success, (default to true for backward compatibility)
2) Extend the check above so if !Success || !string.IsNullOrEmpty(this.ErrorMessage) do what is currently being done

Something like this:

       private bool _success = true;

       public bool Success
       {
           get { return _success; }
           set { _success = value; }
        }

        ... everything else as before ...

        public override void ExecuteResult(ControllerContext context)
        {
            DirectResponse response = new DirectResponse();

            response.Result = this.Result;
            response.IsUpload = this.IsUpload;

            response.Success = ResourceManager.AjaxSuccess;
            response.ErrorMessage = ResourceManager.AjaxErrorMessage;

            if (!this.Success || !string.IsNullOrEmpty(this.ErrorMessage)) // <-- only modification to this method is here
            {
                response.Success = false;
                response.ErrorMessage = this.ErrorMessage;
            }
            else
            {                
                if (!string.IsNullOrEmpty(this.Script))
                {
                    response.Script = this.Script;
                }

                if (this.ExtraParamsResponse.Count > 0)
                {
                    response.ExtraParamsResponse = this.ExtraParamsResponse.ToJson();
                }
            }
            
            response.Return();
        }

Or might this cause some unforeseen issues?

If there might be problems I have not considered, is it possible to refactor out the two lines into a virtual method:

if (!string.IsNullOrEmpty(this.ErrorMessage))

into something like

if (!this.HasError())

... etc ...

protected virtual bool HasError()
{
    return !string.IsNullOrEmpty(this.ErrorMessage)
}

Then I can create a subclass and just override the HasError method and include my own success property in my subclass...

Thanks!

[Daniil]

Hi Anup,

I will investigate your request in details. For now I can say that I needed the Success property a few times as well:)

[Daniil]

Finally, I think it is a lack in API. Our DirectResponse class has the Success property and I don't see any reason why the DirectResult class should not have it.

Created an Issue.
https://github.com/extnet/Ext.NET/issues/611

Added for v2 (trunk) in the revision #6212. It will go to the v2.5.4 release.

I have done the same change for v3 locally, but I will commit later, because currently the v3.0.0 release is under releasing.

[anup]

Many thanks for this. I confirm it works nicely in 2.x (not tested 3.x).

Also, as an aside, I created my own ControllerExtensions class similar to yours in order to add this override of this.Direct:

        public static DirectResult Direct(this Controller controller, bool success, string errorMessage = null)
        {
            return new DirectResult
            {
                Success      = success,
                ErrorMessage = errorMessage
            };
        }

It might be useful to add something similar in your ControllerExtensions maybe?

(My example is probably limited - something better might be one that takes an object result as well as success/errormessage maybe? Or maybe I could have renamed my overload to DirectError, in which case "success" would actually not be needed in the parameter, and just set to false, with the optional error message...)

Anyway, this part is not essential, just an idea... You can close this request. Thanks for the quick turnaround, again. Really appreciated.

[Daniil]

Thank you for the suggestion.

My example is probably limited - something better might be one that takes an object result as well as success/errormessage maybe?

Let's try to come up with some exact thing that you would like to be added. At first glance I doubt about the DirectError class. It looks a bit inconsistent for me.

Except the extension method that you have already provided, what else extensions methods would you like to see built-in?

[anup]

Thanks for the follow up.

At the moment, for me I don't think I need anything further.

I don't know if you have come across other usage scenarios for yourself or others that would require other combinations to be supported?

[Daniil]

Probably, there is an enormous amount of possible combinations:) I think we can add something on demand.

So, am I adding the extension that you proposed, right?

[anup]

Yes. lots of combinations :)

If you think others will benefit from that extension that I added, then yeah, please do so, and I can remove it from my code. Much appreciated!

[Daniil]

Yes, I think it is worth to add.

v2: revision #6220
v3: revision #6219

It will go to v2.5.4 and v3.1.0 releases.