Merge lp:~mabac/launchpad-work-items-tracker/dont-fail-on-valuerror into lp:~linaro-automation/launchpad-work-items-tracker/linaro

Proposed by Mattias Backman on 2011-12-14
Status: Merged
Merged at revision: 315
Proposed branch: lp:~mabac/launchpad-work-items-tracker/dont-fail-on-valuerror
Merge into: lp:~linaro-automation/launchpad-work-items-tracker/linaro
Diff against target: 28 lines (+8/-3)
1 file modified
collect_roadmap (+8/-3)
To merge this branch: bzr merge lp:~mabac/launchpad-work-items-tracker/dont-fail-on-valuerror
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) 2011-12-14 Approve on 2011-12-15
Review via email: mp+85683@code.launchpad.net

Description of the change

Hi,

This branch makes the roadmap card import cope a little better with faulty Papyrs url:s from Kanbantool.

We print a message to the log and continue if the url gives a 404 and do the same if the get_json_data() function raises a ValueError. This should allow the import to continue if someone has either entered a non-existing url or a url to something that is not a json document.

The Papyrs fields of the Card will be blank (None) but the rest of the import can continue.

Thanks,

Mattias

To post a comment you must log in.
Guilherme Salgado (salgado) wrote :

Looks good to me. Just one small suggestion:

 review approve

> differences between files attachment (review-diff.txt)
> === modified file 'collect_roadmap'
> --- collect_roadmap 2011-12-13 09:24:21 +0000
> +++ collect_roadmap 2011-12-14 15:27:54 +0000
> @@ -52,6 +52,8 @@
> print "HTTP error: %d" % e.code
> except urllib2.URLError, e:
> print "Network error: %s" % e.reason.args[1]
> + except ValueError, e:
> + print "Data error: %s" % e.message

Can you print the URL here as well so that people looking at the logs
know what they need to fix?

>
> return data
>
> @@ -161,7 +163,10 @@
> acceptance_criteria = None
>
> page = get_json_data(requirement_url + '?json&auth_token=e8a2801e4f0f')
> - assert page is not None, "Could not access Papyrs document %s." % requirement_url
> + if page is None:
> + return {'description': None,
> + 'acceptance_criteria': None}
> +
> page_text_items = page[0]
> page_extra_items = page[1]
>
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

review: Approve
317. By Mattias Backman on 2011-12-15

Add url to error message.

Mattias Backman (mabac) wrote :

On Thu, Dec 15, 2011 at 1:26 PM, Guilherme Salgado
<email address hidden> wrote:
> Review: Approve
>
> Looks good to me. Just one small suggestion:
>
>  review approve
>
>> differences between files attachment (review-diff.txt)
>> === modified file 'collect_roadmap'
>> --- collect_roadmap   2011-12-13 09:24:21 +0000
>> +++ collect_roadmap   2011-12-14 15:27:54 +0000
>> @@ -52,6 +52,8 @@
>>          print "HTTP error: %d" % e.code
>>      except urllib2.URLError, e:
>>          print "Network error: %s" % e.reason.args[1]
>> +    except ValueError, e:
>> +        print "Data error: %s" % e.message
>
> Can you print the URL here as well so that people looking at the logs
> know what they need to fix?

Sure, I added that for the network related errors too.

>
>>
>>      return data
>>
>> @@ -161,7 +163,10 @@
>>      acceptance_criteria = None
>>
>>      page = get_json_data(requirement_url + '?json&auth_token=e8a2801e4f0f')
>> -    assert page is not None, "Could not access Papyrs document %s." % requirement_url
>> +    if page is None:
>> +        return {'description': None,
>> +                'acceptance_criteria': None}
>> +
>>      page_text_items = page[0]
>>      page_extra_items = page[1]
>>
>>
>
> --
> Guilherme Salgado <https://launchpad.net/~salgado>
>
> https://code.launchpad.net/~mabac/launchpad-work-items-tracker/dont-fail-on-valuerror/+merge/85683
> You are the owner of lp:~mabac/launchpad-work-items-tracker/dont-fail-on-valuerror.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'collect_roadmap'
2--- collect_roadmap 2011-12-13 09:24:21 +0000
3+++ collect_roadmap 2011-12-15 13:06:28 +0000
4@@ -49,9 +49,11 @@
5 try:
6 data = simplejson.load(urllib2.urlopen(url))
7 except urllib2.HTTPError, e:
8- print "HTTP error: %d" % e.code
9+ print "HTTP error for url '%s': %d" % (url, e.code)
10 except urllib2.URLError, e:
11- print "Network error: %s" % e.reason.args[1]
12+ print "Network error for url '%s': %s" % (url, e.reason.args[1])
13+ except ValueError, e:
14+ print "Data error for url '%s': %s" % (url, e.message)
15
16 return data
17
18@@ -161,7 +163,10 @@
19 acceptance_criteria = None
20
21 page = get_json_data(requirement_url + '?json&auth_token=e8a2801e4f0f')
22- assert page is not None, "Could not access Papyrs document %s." % requirement_url
23+ if page is None:
24+ return {'description': None,
25+ 'acceptance_criteria': None}
26+
27 page_text_items = page[0]
28 page_extra_items = page[1]
29

Subscribers

People subscribed via source and target branches