Merge lp:~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api into lp:ubuntu-weather-app

Proposed by Victor Thompson on 2016-03-20
Status: Merged
Approved by: Victor Thompson on 2016-06-05
Approved revision: 237
Merged at revision: 242
Proposed branch: lp:~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api
Merge into: lp:ubuntu-weather-app
Diff against target: 395 lines (+167/-91)
4 files modified
app/data/WeatherApi.js (+151/-87)
app/ui/LocationPane.qml (+1/-1)
debian/changelog (+14/-2)
manifest.json.in (+1/-1)
To merge this branch: bzr merge lp:~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api
Reviewer Review Type Date Requested Status
Jenkins Bot continuous-integration Approve on 2016-06-08
Victor Thompson Approve on 2016-06-05
Andrew Hayzen Needs Information on 2016-06-04
Review via email: mp+289584@code.launchpad.net

Commit message

* Update to use new weather API
* Change the trimAPIKey function so it only trims the API Key
* Release 3.2 and bump version to 3.3

Description of the change

Move to the new weather api.

For reference, here is the commit to use the new API in the scope: http://bazaar.launchpad.net/~ubuntuone-hackers/ubuntu-rest-scopes/trunk/revision/505

To post a comment you must log in.
Andrew Hayzen (ahayzen) wrote :

OK, this is the first iteration of a 'working' solution, still needs lots of testing :-) Basically as TWC now, from what I understand, requires us to do multiple url calls to get the current, daily and hourly data. I've copied the OWM code and modified it to match the TWC api (as OWM is similar in the fact that it requires multiple url calls).

Main inline comment so far is:
Not sure what todo here? This is for "weather codes" such as, UKXX0085, which we were using, but I cannot see that in the new API? Currently the 1==2 is forcing it to use coordinate based lookups.

Also was the search using TWC? I need to check if that needs to be updated?

review: Approve (continuous-integration)
Victor Thompson (vthompson) wrote :

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

review: Needs Fixing
233. By Andrew Hayzen on 2016-06-04

* Release 3.2 and update changelog

234. By Andrew Hayzen on 2016-06-04

* Comment out 'dead code' for now

review: Approve (continuous-integration)
Andrew Hayzen (ahayzen) wrote :

1) Done
2) Done bumped the version and put the relevant stuff in the sections
3) I've commented out the 'dead code' where it uses the location code for TWC, let me know if you want me to remove

Also, I've spotted one todo on line 44 of the diff
+ var partData = dataObj["metric"] || dataObj["imperial"] || dataObj; // TODO: be more intelligent?

Do you think this is OK? or should it be more like?
if (there is metric) { use metric } else if (there is imperial) { use imperial } else { fallback }

Futhermore the lines below have things like calcFahrenheit() which convert from metric->imperial, so it assumes the data is metric, maybe we should remove the imperial option... so just

+ var partData = dataObj["metric"] || dataObj;

I think i've talked myself into changing it to that :')

review: Needs Information
235. By Andrew Hayzen on 2016-06-04

* Remove dead code
* Fix up imperial todo

review: Approve (continuous-integration)
Victor Thompson (vthompson) wrote :

This isn't a regression, as I've noticed it before--TWC always displays the high temp as the current temp as soon as the high has been reached. Maybe we're using the API incorrectly to determine today's high? Since it's not a regression, I'm fine leaving it as-is... but we should probably file a bug or add a comment to the code with a TODO to investigate. Bug would probably be preferred.

review: Needs Information
Victor Thompson (vthompson) wrote :

Actually, maybe it's not a bug. Weather.com seems to do the same. Maybe that's their way of indicating that the temp won't rise?

236. By Andrew Hayzen on 2016-06-04

* Fix for max_temp being null in TWC causing 'high' to be 0 and therefore lower than the 'low', now show a "-"

review: Approve (continuous-integration)
237. By Andrew Hayzen on 2016-06-04

* Use "en dash"

review: Approve (continuous-integration)
Victor Thompson (vthompson) wrote :

Ok, this looks good now. Approve!

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/data/WeatherApi.js'
2--- app/data/WeatherApi.js 2015-10-21 02:16:50 +0000
3+++ app/data/WeatherApi.js 2016-06-04 21:12:20 +0000
4@@ -100,15 +100,20 @@
5 }
6
7
8-// Remove anything including and after APPID in the given term
9+// Remove just the API key
10 function trimAPIKey(data) {
11- var owm = data.indexOf("&APPID=");
12- var twc = data.indexOf("&key=");
13+ var owm = data.indexOf("APPID=");
14+ var twc = data.indexOf("apiKey=");
15+
16+ // Key length is 32 char for both APIs
17+ var keySize = 32;
18
19 if (owm > -1) {
20- data = data.substr(0, owm);
21+ var replace = data.substr(owm+6, keySize);
22+ data = data.replace(replace,"")
23 } else if (twc > -1) {
24- data = data.substr(0, twc);
25+ var replace = data.substr(twc+7, keySize);
26+ data = data.replace(replace,"")
27 }
28
29 return data;
30@@ -400,7 +405,7 @@
31 /**
32 provides neccessary methods for requesting and preparing data from OpenWeatherMap.org
33 */
34- var _baseUrl = "http://wxdata.weather.com/wxdata/";
35+ var _baseUrl = "https://api.weather.com/";
36 //
37 var _serviceName = "weatherchannel";
38 //
39@@ -457,65 +462,70 @@
40 };
41 //
42 function _buildDataPoint(date, dataObj) {
43- var data = dataObj["Observation"] || dataObj,
44+ var partData = dataObj["metric"] || dataObj;
45+ var data = dataObj["observation"] || dataObj,
46 result = {
47- timestamp: data.date || data.dateTime,
48+ timestamp: data.fcst_valid,
49 date: date,
50 metric: {
51- temp: data.temp,
52- tempFeels: data.feelsLike,
53- windSpeed: data.wSpeed
54+ temp: partData.temp,
55+ tempFeels: partData.feels_like,
56+ windSpeed: partData.wspd
57 },
58 imperial: {
59- temp: calcFahrenheit(data.temp),
60- tempFeels: calcFahrenheit(data.feelsLike),
61- windSpeed: convertKphToMph(data.wSpeed)
62+ temp: calcFahrenheit(partData.temp),
63+ tempFeels: calcFahrenheit(partData.feels_like),
64+ windSpeed: convertKphToMph(partData.wspd)
65 },
66 precipType: (data.precip_type !== undefined) ? data.precip_type : null,
67 propPrecip: (data.pop !== undefined) ? data.pop : null,
68- humidity: data.humid,
69- pressure: data.pressure,
70- windDeg: data.wDir,
71- windDir: data.wDirText,
72- icon: _iconMap[(data.wxIcon||data.icon)],
73- condition: data.text || data.wDesc,
74- uv: data.uv
75+ humidity: partData.rh,
76+ pressure: partData.mslp,
77+ windDeg: data.wdir,
78+ windDir: data.wdir_cardinal,
79+ icon: _iconMap[data.icon_code],
80+ condition: data.phrase_32char,
81+ uv: data.uv_index
82 };
83- if(_iconMap[data.wxIcon||data.icon] === undefined) {
84- print("ICON MISSING POINT: "+(data.wxIcon||data.icon)+" "+result.condition)
85+
86+ if (_iconMap[data.icon_code] === undefined) {
87+ print("ICON MISSING POINT: " + data.icon_code + " " + result.condition)
88 }
89+
90 return result;
91 }
92 //
93 function _buildDayFormat(date, data, now) {
94- var partData = (now > data.validDate || data.day === undefined) ? data.night : data.day,
95+ var partData = (now > data.fcst_valid || data.day === undefined) ? data.night : data.day,
96 result = {
97 date: date,
98- timestamp: data.validDate,
99+ timestamp: data.fcst_valid,
100 metric: {
101- tempMin: data.minTemp,
102- tempMax: data.maxTemp,
103- windSpeed: partData.wSpeed
104+ tempMin: data.min_temp,
105+ tempMax: data.max_temp !== null ? data.max_temp : undefined,
106+ windSpeed: partData.wspd
107 },
108 imperial: {
109- tempMin: calcFahrenheit(data.minTemp),
110- tempMax: calcFahrenheit(data.maxTemp !== undefined ? data.maxTemp : data.minTemp),
111- windSpeed: convertKphToMph(partData.wSpeed)
112+ tempMin: calcFahrenheit(data.min_temp),
113+ tempMax: data.max_temp !== null && data.max_temp !== undefined ? calcFahrenheit(data.max_temp) : undefined,
114+ windSpeed: convertKphToMph(partData.wspd)
115 },
116 precipType: partData.precip_type,
117 propPrecip: partData.pop,
118 pressure: null,
119- humidity: partData.humid,
120- icon: _iconMap[partData.icon],
121- condition: partData.phrase,
122- windDeg: partData.wDir,
123- windDir: partData.wDirText,
124- uv: partData.uv,
125+ humidity: partData.rh,
126+ icon: _iconMap[partData.icon_code],
127+ condition: partData.phrase_32char,
128+ windDeg: partData.wdir,
129+ windDir: partData.wdir_cardinal,
130+ uv: partData.uv_index,
131 hourly: []
132 }
133- if(_iconMap[partData.icon] === undefined) {
134- print("ICON MISSING DAY: "+partData.icon+" "+result.condition)
135+
136+ if (_iconMap[partData.icon_code] === undefined) {
137+ print("ICON MISSING DAY: " + partData.icon_code + " " + result.condition)
138 }
139+
140 return result;
141 }
142 //
143@@ -527,35 +537,43 @@
144 nowMs = parseInt(now/1000),
145 localNow = getLocationTime(now+offset),
146 data = {
147- "location": combinedData[0]["Location"],
148- "daily": combinedData[0]["DailyForecasts"],
149- "forecast": combinedData[0]["HourlyForecasts"],
150- "current": combinedData[0]["StandardObservation"],
151- "sunRiseSet": combinedData[0]["SunRiseSet"],
152+ "location": combinedData["current"]["metadata"],
153+ "daily": combinedData["daily"]["forecasts"],
154+ "forecast": combinedData["forecast"]["forecasts"],
155+ "current": combinedData["current"]["observation"],
156 };
157 print("["+location.name+"] "+JSON.stringify(localNow));
158 // add openweathermap id for faster responses
159 if(location.services && !location.services[_serviceName] && data["location"].key) {
160 location.services[_serviceName] = data["location"].key
161- }
162+ }
163 // only 5 days of forecast for TWC
164- for(var x=0;x<5;x++) {
165+ for(var x=0; x<5; x++) {
166 var dayData = data["daily"][x],
167- date = getLocationTime(((dayData.validDate*1000)-1000)+offset); // minus 1 sec to handle +/-12 TZ
168- var sunRiseSet = data["sunRiseSet"][x];
169+ date = getLocationTime(((dayData.fcst_valid * 1000) - 1000) + offset); // minus 1 sec to handle +/-12 TZ
170+
171+ // Sun{rise,set} is in ISOString format so use getTime() to convert
172+ var sunrise = new Date(dayData.sunrise).getTime(),
173+ sunset = new Date(dayData.sunset).getTime();
174 day = date.year+"-"+date.month+"-"+date.date;
175- if(!todayDate) {
176- if(localNow.year+"-"+localNow.month+"-"+localNow.date > day) {
177+
178+ if (!todayDate) {
179+ if (localNow.year + "-" + localNow.month + "-" + localNow.date > day) {
180 // skip "yesterday"
181 continue;
182 }
183+
184 todayDate = date;
185 }
186+
187 tmpResult[day] = _buildDayFormat(date, dayData, nowMs);
188+
189 var timezoneOffset = new Date().getTimezoneOffset();
190 var timesOffset = (location.timezone && location.timezone.dstOffset !== undefined) ? (location.timezone.dstOffset*60 + timezoneOffset)*60*1000: 0
191- var sunrise = new Date(sunRiseSet.rise*1000 + timesOffset);
192- var sunset = new Date(sunRiseSet.set*1000 + timesOffset);
193+
194+ sunrise = new Date(sunrise + timesOffset);
195+ sunset = new Date(sunset + timesOffset);
196+
197 var options = { timeZone: location.timezone.timeZoneId, timeZoneName: 'long' };
198 tmpResult[day].sunrise = sunrise.toLocaleTimeString(Qt.locale().name, options);
199 tmpResult[day].sunset = sunset.toLocaleTimeString(Qt.locale().name, options);
200@@ -563,8 +581,9 @@
201 //
202 if(data["forecast"] !== undefined) {
203 data["forecast"].forEach(function(hourData) {
204- var dateData = getLocationTime((hourData.dateTime*1000)+offset),
205+ var dateData = getLocationTime((hourData.fcst_valid * 1000) + offset),
206 day = dateData.year+"-"+dateData.month+"-"+dateData.date;
207+
208 if(tmpResult[day]) {
209 tmpResult[day]["hourly"].push(_buildDataPoint(dateData, hourData));
210 }
211@@ -591,48 +610,93 @@
212 return result;
213 }
214 //
215- function _getUrl(params) {
216- var url, serviceId,
217- baseParams = {
218- key: params.twc_api_key,
219- units: (params.units === "metric") ? "m" : "e",
220- locale: Qt.locale().name,
221- hours: "48",
222- },
223- commands = {
224- "mobileaggregation": "mobile/mobagg/",
225+ function _getUrls(params) {
226+ var baseParams = {
227+ units: (params.units === "metric") ? "m" : "e",
228+ language: Qt.locale().name.replace("_","-"),
229+ apiKey: params.twc_api_key,
230+ };
231+ var commands = {
232+ "geocode": "v1/geocode/",
233+ };
234+ var urls = {
235+ current: "",
236+ daily: "",
237+ hourly: "",
238+ };
239+
240+ // FIXME: only use coords for now and not location codes (UKXX0085)
241+ if (params.location.coord) {
242+ var coord = {
243+ lat: params.location.coord.lat,
244+ lng: params.location.coord.lon
245 };
246- if(params.location.services && params.location.services[_serviceName]) {
247- serviceId = encodeURIComponent(params.location.services[_serviceName]);
248- url = _baseUrl+commands["mobileaggregation"]+serviceId+".js?"+parameterize(baseParams);
249- } else if (params.location.coord) {
250- var coord = {lat: params.location.coord.lat, lng: params.location.coord.lon};
251- url = _baseUrl+commands["mobileaggregation"]+"get.js?"+parameterize(baseParams)+"&"+
252- parameterize(coord);
253+
254+ urls.current = _baseUrl + commands["geocode"] +
255+ coord.lat + "/" + coord.lng +
256+ "/observations/current.json?" +
257+ parameterize(baseParams);
258+ urls.daily = _baseUrl + commands["geocode"] +
259+ coord.lat + "/" + coord.lng +
260+ "/forecast/daily/5day.json?" +
261+ parameterize(baseParams);
262+ urls.hourly = _baseUrl + commands["geocode"] +
263+ coord.lat + "/" + coord.lng +
264+ "/forecast/hourly/48hour.json?" +
265+ parameterize(baseParams);
266 }
267- return url;
268+
269+ return urls;
270 }
271 //
272 return {
273 getData: function(params, apiCaller, onSuccess, onError) {
274- var url = _getUrl(params),
275+ var urls = _getUrls(params),
276 handlerMap = {
277- all: { type: "all", url: url}
278- },
279- response = {
280- location: params.location,
281- db: (params.db) ? params.db : null,
282- format: RESPONSE_DATA_VERSION
283- },
284- addDataToResponse = (function(request, data) {
285- var formattedResult;
286- response["data"] = formatResult(data, params.location);
287+ current: {
288+ type: "current",
289+ url: urls.current
290+ },
291+ daily: {
292+ type: "daily",
293+ url: urls.daily
294+ },
295+ forecast: {
296+ type: "forecast",
297+ url: urls.hourly
298+ }
299+ },
300+ response = {
301+ location: params.location,
302+ db: (params.db) ? params.db : null,
303+ format: RESPONSE_DATA_VERSION
304+ },
305+ respData = {},
306+ addDataToResponse = (function(request, data) {
307+ var formattedResult;
308+ respData[request.type] = data;
309+
310+ if (respData["current"] !== undefined &&
311+ respData["forecast"] !== undefined &&
312+ respData["daily"] !== undefined) {
313+
314+ response["data"] = formatResult(respData, params.location);
315 onSuccess(response);
316- }),
317- onErrorHandler = (function(err) {
318- onError(err);
319- });
320- apiCaller(handlerMap.all, addDataToResponse, onErrorHandler);
321+ }
322+ }),
323+ onErrorHandler = (function(err) {
324+ onError(err);
325+ }),
326+ retryHandler = (function(err) {
327+ console.log("retry of " + trimAPIKey(err.request.url));
328+ var retryFunc = handlerMap[err.request.type];
329+
330+ apiCaller(retryFunc, addDataToResponse, onErrorHandler);
331+ });
332+
333+ apiCaller(handlerMap.current, addDataToResponse, retryHandler);
334+ apiCaller(handlerMap.forecast, addDataToResponse, retryHandler);
335+ apiCaller(handlerMap.daily, addDataToResponse, retryHandler);
336 }
337 }
338 })();
339
340=== modified file 'app/ui/LocationPane.qml'
341--- app/ui/LocationPane.qml 2016-02-06 01:17:07 +0000
342+++ app/ui/LocationPane.qml 2016-06-04 21:12:20 +0000
343@@ -150,7 +150,7 @@
344 return {
345 day: formatTimestamp(data.date, 'dddd'),
346 low: Math.round(data[tempUnits].tempMin).toString() + settings.tempScale,
347- high: (data[tempUnits].tempMax !== undefined) ? Math.round(data[tempUnits].tempMax).toString() + settings.tempScale : "",
348+ high: (data[tempUnits].tempMax !== undefined) ? Math.round(data[tempUnits].tempMax).toString() + settings.tempScale : "–",
349 image: (data.icon !== undefined && iconMap[data.icon] !== undefined) ? iconMap[data.icon] : "",
350 condition: emptyIfUndefined(data.condition),
351 chanceOfPrecip: emptyIfUndefined(data.propPrecip, "%"),
352
353=== modified file 'debian/changelog'
354--- debian/changelog 2016-03-26 16:38:54 +0000
355+++ debian/changelog 2016-06-04 21:12:20 +0000
356@@ -1,4 +1,16 @@
357-ubuntu-weather-app (3.2) UNRELEASED; urgency=medium
358+ubuntu-weather-app (3.3ubuntu1) UNRELEASED; urgency=medium
359+
360+ [ Andrew Hayzen ]
361+ * Release 3.2 and bump version to 3.3
362+ * Update to use new weather API
363+
364+ [ Victor Thompson ]
365+ * Update to use new weather API
366+ * Change the trimAPIKey function so it only trims the API Key
367+
368+ -- Andrew Hayzen <ahayzen@gmail.com> Sat, 04 Jun 2016 18:31:29 +0100
369+
370+ubuntu-weather-app (3.2ubuntu1) vivid; urgency=medium
371
372 [ Andrew Hayzen ]
373 * Release 3.1 and bump version to 3.2
374@@ -20,7 +32,7 @@
375 [ Vamshi Balanaga ]
376 * Added test to swipe between location tabs on home page (LP: #1452492)
377
378- -- Andrew Hayzen <ahayzen@gmail.com> Thu, 03 Dec 2015 15:20:49 +0000
379+ -- Andrew Hayzen <ahayzen@gmail.com> Sat, 04 Jun 2016 18:30:00 +0100
380
381 ubuntu-weather-app (3.1ubuntu1) vivid; urgency=medium
382
383
384=== modified file 'manifest.json.in'
385--- manifest.json.in 2016-02-28 22:15:28 +0000
386+++ manifest.json.in 2016-06-04 21:12:20 +0000
387@@ -12,7 +12,7 @@
388 "maintainer": "Ubuntu App Cats <ubuntu-touch-coreapps@lists.launchpad.net>",
389 "name": "@PROJECT_NAME@",
390 "title": "Weather",
391- "version": "3.2.@BZR_REVNO@",
392+ "version": "3.3.@BZR_REVNO@",
393 "x-source": {
394 "vcs-bzr": "@BZR_SOURCE@",
395 "vcs-bzr-revno": "@BZR_REVNO@"

Subscribers

People subscribed via source and target branches