U4-9489 - GetUserLog of angularResource logResource is broken for date filtering

Created by Marc Goodson 04 Feb 2017, 18:29:09 Updated by Marc Goodson 20 Jun 2017, 22:29:58

It's becuse the umbraco.services umbRequestHelper dictionaryToQueryString helper method only returns the first item when passed an array of several parameters...

For example in the getUserLog method of the logResource the following request is made to the api endpoint:

umbRequestHelper.getApiUrl( "logApiBaseUrl", "GetCurrentUserLog", [{ logtype: type, sinceDate: since }])

...however only the logType parameter is passed to the api, not the sinceDate, if you change the order eg:

umbRequestHelper.getApiUrl( "logApiBaseUrl", "GetCurrentUserLog", [{ sinceDate: since , logtype: type }])

then only the sinceDate is passed and not logtype !!!

Following the request through to the getApiUrl helper

this uses the following code to return the api url:

return Umbraco.Sys.ServerVariables["umbracoUrls"][apiName] + actionName + (!queryStrings ? "" : "?" + (angular.isString(queryStrings) ? queryStrings : this.dictionaryToQueryString(queryStrings)));

since what is being passed in is not a string but instead an object, the dictionaryToQueryString helper gets called:

if (angular.isArray(queryStrings)) { return _.map(queryStrings, function (item) { var key = null; var val = null; for (var k in item) { key = k; val = item[k]; break; } if (key === null || val === null) { throw "The object in the array was not formatted as a key/value pair"; } return encodeURIComponent(key) + "=" + encodeURIComponent(val); }).join("&");

which is trying to loop through each key value pair in the object into a flat encoded querystring of the parameters.

'''But if you look in the 'for' loop there is a break; !!!!''' so consequently only the first item in the object is ever parsed and the rest is ignored. (unless it's a problem with _.map not handling the angular array correctly ? and the break 'should be there' but for some reason we're not getting multiple items))

This prevents the getUserLog method from returning anything in the logResource as the sinceDate parameter is missed off and so consequently doesn't match the signature of the api controller.

This is hard to spot as it's fine when there is only one parameter... ... and also when the get request fails, both the getUserLog and getLog error anyway because their error message contains a variable that isn't declared: getLog: function (type, since) {
return umbRequestHelper.resourcePromise( $http.get( umbRequestHelper.getApiUrl( "logApiBaseUrl", "GetLog", [{ logtype: type, sinceDate: since }])), 'Failed to retrieve user data for id ' + id); }

the id variable doesn't exist in these two methods

I don't know enough about why the '''break;''' was inserted or where else this code is called, (and fixing it might therefore break something else) but suggested fix would be something like this:

if (angular.isArray(queryStrings)) { return _.map(queryStrings, function (item) { var key = null; var val = null; var encodedQueryStrings = []; // can be multiple parameters passed via array for (var k in item) { key = k; val = item[k]; encodedQueryStrings.push(encodeURIComponent(key) + "=" + encodeURIComponent(val)); } if (key === null || val === null) { throw "The object in the array was not formatted as a key/value pair"; }
return encodedQueryStrings.join("&"); }).join("&"); this way each passed item in the object gets concatenated into an encoded querystring - but there maybe a more elegant fix ?

putting this in has enabled me to pull back user log data from the logResource... I haven't noticed any other ill effects.

Comments

Marc Goodson 04 Feb 2017, 18:47:42

Pull Request: https://github.com/umbraco/Umbraco-CMS/pull/1729


Shannon Deminick 31 May 2017, 12:56:23

The work around is to use : { logtype: type, sinceDate: since } instead of [{ logtype: type, sinceDate: since }]

but of course this should still work


Shannon Deminick 31 May 2017, 13:24:14

Actually, the way it was meant to work is either:

{ logtype: type, sinceDate: since }

or

[{ logtype: type}, {sinceDate: since }]

So either a dictionary or an array of key value pairs, this was never intended to work:

[{ logtype: type, sinceDate: since }]

which is an array of a single dictionary


Marc Goodson 02 Jun 2017, 11:14:41

@Shandem so is it better to fix the logResource itself to pass the parameters correctly https://github.com/umbraco/Umbraco-CMS/blob/145f7896316f8ead11a3492aadc848114ea42b76/src/Umbraco.Web.UI.Client/src/common/resources/log.resource.js or the fix above to cope with them being passed in this manner?


Marc Goodson 02 Jun 2017, 11:15:24

@Shandem ahh actually I see, you've pulled it in :-)


Shannon Deminick 02 Jun 2017, 11:25:58

@marcemarc i pulled it in and reverted it ;)

I forgot one of our own services was using it wrong... so yes, the correct way to fix it is to make sure it's passing the parameters correct. Any chance you could send a PR?


Marc Goodson 02 Jun 2017, 20:18:32

@Shandem something like this ? https://github.com/umbraco/Umbraco-CMS/pull/1973


Shannon Deminick 19 Jun 2017, 07:44:21

Great!


Marc Goodson 20 Jun 2017, 22:29:58

@Shandem thanks!


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Easy

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.5.10

Due in version: 7.6.4

Sprint:

Story Points:

Cycle: 2