ubuntu-touch-coreapps-reviewers team mailing list archive

[Victor Thompson <victor.thompson@xxxxxxxxx>]

Review: Needs Fixing

Thanks for finishing this off, Andrew!!

I ran AP and all tests pass. Code changes look fine so far, but one thing we might want to take the opportunity to do now (otherwise we may never do so) is to audit our code coverage in WeatherApi.js and remove any dead code so we don't do needless maintenance on unused code in the future. QtCreator doesn't seem to have a coverage tool, but there seem to be numerous third party tools and qml plugins that might be helpful. Maybe just a JS only coverage tool would be sufficient.

I've been testing this change out and have not hit any regressions thus far. Unless we want to look at removing any dead code, I think this is ready for QA.

TODO:
1. Update changelog
2. Bump version number?
3. Potentially look at removing any dead code

Diff comments:

> === modified file 'app/data/WeatherApi.js'
> --- app/data/WeatherApi.js	2015-10-21 02:16:50 +0000
> +++ app/data/WeatherApi.js	2016-05-28 00:41:30 +0000
> @@ -591,48 +610,101 @@
>          return result;
>      }
>      //
> -    function _getUrl(params) {
> -        var url, serviceId,
> -            baseParams = {
> -                key: params.twc_api_key,
> -                units: (params.units === "metric") ? "m" : "e",
> -                locale: Qt.locale().name,
> -                hours: "48",
> -            },
> -            commands = {
> -                "mobileaggregation": "mobile/mobagg/",
> +    function _getUrls(params) {
> +        var baseParams = {
> +            units: (params.units === "metric") ? "m" : "e",
> +            language: Qt.locale().name.replace("_","-"),
> +            apiKey: params.twc_api_key,
> +        };
> +        var commands = {
> +            "geocode": "v1/geocode/",
> +        };
> +        var urls = {
> +            current: "",
> +            daily: "",
> +            hourly: "",
> +        };
> +
> +        if(1==2 && params.location.services && params.location.services[_serviceName]) {  // FIXME: disable for now (UKXX0085)

We'll have to look into it more, but it looks like we could remove this dead code and always encode the lat/long

> +            var serviceId = encodeURIComponent(params.location.services[_serviceName]);
> +
> +            urls.current = _baseUrl + commands["geocode"] +
> +                    serviceId + ".js?" + parameterize(baseParams);
> +            urls.daily = _baseUrl + commands["geocode"] +
> +                    serviceId + ".js?" + parameterize(baseParams);
> +            urls.hourly = _baseUrl + commands["geocode"] +
> +                    serviceId + ".js?" + parameterize(baseParams);
> +        } else if (params.location.coord) {
> +            var coord = {
> +                lat: params.location.coord.lat,
> +                lng: params.location.coord.lon
>              };
> -        if(params.location.services && params.location.services[_serviceName]) {
> -            serviceId = encodeURIComponent(params.location.services[_serviceName]);
> -            url = _baseUrl+commands["mobileaggregation"]+serviceId+".js?"+parameterize(baseParams);
> -        } else if (params.location.coord) {
> -            var coord = {lat: params.location.coord.lat, lng: params.location.coord.lon};
> -            url = _baseUrl+commands["mobileaggregation"]+"get.js?"+parameterize(baseParams)+"&"+
> -                  parameterize(coord);
> +
> +            urls.current = _baseUrl + commands["geocode"] +
> +                    coord.lat + "/" + coord.lng +
> +                    "/observations/current.json?" +
> +                    parameterize(baseParams);
> +            urls.daily = _baseUrl + commands["geocode"] +
> +                    coord.lat + "/" + coord.lng +
> +                    "/forecast/daily/5day.json?" +
> +                    parameterize(baseParams);
> +            urls.hourly = _baseUrl + commands["geocode"] +
> +                    coord.lat + "/" + coord.lng +
> +                    "/forecast/hourly/48hour.json?" +
> +                    parameterize(baseParams);
>          }
> -        return url;
> +
> +        return urls;
>      }
>      //
>      return {
>          getData: function(params, apiCaller, onSuccess, onError) {
> -            var url = _getUrl(params),
> +            var urls = _getUrls(params),
>                  handlerMap = {
> -                    all: { type: "all", url: url}
> -                },
> -                response = {
> -                    location: params.location,
> -                    db: (params.db) ? params.db : null,
> -                    format: RESPONSE_DATA_VERSION
> -                },
> -                addDataToResponse = (function(request, data) {
> -                    var formattedResult;
> -                    response["data"] = formatResult(data, params.location);
> +                current: {
> +                    type: "current",
> +                    url: urls.current
> +                },
> +                daily: {
> +                    type: "daily",
> +                    url: urls.daily
> +                },
> +                forecast: {
> +                    type: "forecast",
> +                    url: urls.hourly
> +                }
> +            },
> +            response = {
> +                location: params.location,
> +                db: (params.db) ? params.db : null,
> +                format: RESPONSE_DATA_VERSION
> +            },
> +            respData = {},
> +            addDataToResponse = (function(request, data) {
> +                var formattedResult;
> +                respData[request.type] = data;
> +
> +                if (respData["current"] !== undefined &&
> +                        respData["forecast"] !== undefined &&
> +                            respData["daily"] !== undefined) {
> +
> +                    response["data"] = formatResult(respData, params.location);
>                      onSuccess(response);
> -                }),
> -                onErrorHandler = (function(err) {
> -                    onError(err);
> -                });
> -            apiCaller(handlerMap.all, addDataToResponse, onErrorHandler);
> +                }
> +            }),
> +            onErrorHandler = (function(err) {
> +                onError(err);
> +            }),
> +            retryHandler = (function(err) {
> +                console.log("retry of " + trimAPIKey(err.request.url));
> +                var retryFunc = handlerMap[err.request.type];
> +
> +                apiCaller(retryFunc, addDataToResponse, onErrorHandler);
> +            });
> +
> +            apiCaller(handlerMap.current, addDataToResponse, retryHandler);
> +            apiCaller(handlerMap.forecast, addDataToResponse, retryHandler);
> +            apiCaller(handlerMap.daily, addDataToResponse, retryHandler);
>          }
>      }
>  })();


-- 
https://code.launchpad.net/~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api/+merge/289584
Your team Ubuntu Weather Developers is subscribed to branch lp:~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api.