-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
btw: labadmin and amgut nosetests pass on my local machine |
Ready for review @wasade @josenavas |
I need some help with one more utf-8 problem: AN ERROR HAS OCCURED! Please copy the following into an email and send this information, along with the url you were trying to access, to... someone in the knight lab 'ascii' codec can't decode byte 0xc5 in position 100: ordinal not in range(128) Traceback (most recent call last): body: barcodes=000067543%2C000067446%2C000067613%2C000067605%2C000067441%2C000067621%2C000067538%2C000067445%2C000067558%2C000067756%2C000067802%2C000067505%2C000067877%2C000067458%2C000067717%2C000067461%2C000067690%2C000067724%2C000067693%2C000067442%2C000067665%2C000067797%2C000067489%2C000067597%2C000067633%2C000067548%2C000067443%2C000067549%2C000067557%2C000067672%2C000067620%2C000067694%2C000067531%2C000067685%2C000067450%2C000067525%2C000067754%2C000067485%2C000067517%2C000067449%2C000067709%2C000067667%2C000067457%2C000067518%2C000067673%2C000067761%2C000067721%2C000067553%2C000067785%2C000067463%2C000067486%2C000067733%2C000067544%2C000067623%2C000067686%2C000067697%2C000067460%2C000067650%2C000067451%2C000067550%2C000067657%2C000067506%2C000067770%2C000067646%2C000067649%2C000067545%2C000067841%2C000067789%2C000067554%2C000067705%2C000067593%2C000067469%2C000067537%2C000067622%2C000067826%2C000067551%2C000067595%2C000067696%2C000067710%2C000067594%2C000067530%2C000067490%2C000067817%2C000067695%2C000067492%2C000067462%2C000067635%2C000067600%2C000067821%2C000067470%2C000067914%2C000067706%2C000067755%2C000067771%2C000068059%2C000067836%2C000067793%2C000067827%2C000067542%2C000067464%2C000067519%2C000067930%2C000067718%2C000067818%2C000067680%2C000067653%2C000067658%2C000067448%2C000067711%2C000067865%2C000067997%2C000067722%2C000067842%2C000067798%2C000067618%2C000067566%2C000067559%2C000067444%2C000067933%2C000067507%2C000067879%2C000067822%2C000068009%2C000067960%2C000067526%2C000067601%2C000067655%2C000067878%2C000067471%2C000067674%2C000067679%2C000067556%2C000067671%2C000067675%2C000067790%2C000067668%2C000067819%2C000067487%2C000067606%2C000067596%2C000067447%2C000067767%2C000067772%2C000067688%2C000068100%2C000067540%2C000067659%2C000067871%2C000067634%2C000068013%2C000067691%2C000067452%2C000067514%2C000068149%2C000067996%2C000067678%2C000067786%2C000067753%2C000067677%2C000067681%2C000067687%2C000067541%2C000067459%2C000067496%2C000067669%2C000067624%2C000067477%2C000067825%2C000067599%2C000067799%2C000068021%2C000067598%2C000067565%2C000067647%2C000067945%2C000067791%2C000067527%2C000067768%2C000067636%2C000067787%2C000067714%2C000067925%2C000067850%2C000067779%2C000067555%2C000067792%2C000067617%2C000067712%2C000067973%2C000068133%2C000067852%2C000067478%2C000067795%2C000068126%2C000067571%2C000067861%2C000067810%2C000067809%2C000067682%2C000067917%2C000068233%2C000068002%2C000068145%2C000067552%2C000067983%2C000067938%2C000067952%2C000067942%2C000067648%2C000067887%2C000067666%2C000067857%2C000067941%2C000067739%2C000067949%2C000067937%2C000067494%2C000067916%2C000067991%2C000067961%2C000067769%2C000067749%2C000068036%2C000067813%2C000067863%2C000067567%2C000067843%2C000067737%2C000068150%2C000067974%2C000067699%2C000068140%2C000067715%2C000067959%2C000067980%2C000068125%2C000067676%2C000068097%2C000067870%2C000067652%2C000067986%2C000067806%2C000069300%2C000067488%2C000067934%2C000067751%2C000067645%2C000067607%2C000068089%2C000067910%2C000067935%2C000068132%2C000067805%2C000067568%2C000067637%2C000067716%2C000067651%2C000068093%2C000067491%2C000068134%2C000067766%2C000068037%2C000068116%2C000067608%2C000068085%2C000068018%2C000067918%2C000067993%2C000067735%2C000067723%2C000068041%2C000067736%2C000067849%2C000068069%2C000067990%2C000067987%2C000067683%2C000067708%2C000068023%2C000068113%2C000067727%2C000067969%2C000067932%2C000067744%2C000067988%2C000067977%2C000067992%2C000068057%2C000068130%2C000067946%2C000068073%2C000067654%2C000067905%2C000067820%2C000068114%2C000068137%2C000067926%2C000067989%2C000068153%2C000067692%2C000067833%2C000067719%2C000069299%2C000067707%2C000067638%2C000067748%2C000067780%2C000067880%2C000067982%2C000067656%2C000068011%2C000068017%2C000067603%2C000067828%2C000067998%2C000067734%2C000067985%2C000067886%2C000067936%2C000067907%2C000067869%2C000067972%2C000067508%2C000067713%2C000067823%2C000067750%2C000067725%2C000067844%2C000067700%2C000067984%2C000068075%2C000067800%2C000067881%2C000068129%2C000067604%2C000068022%2C000068010%2C000068060%2C000068127%2C000067858%2C000068146%2C000068019%2C000067720%2C000068001%2C000067995%2C000067560%2C000068014%2C000067834%2C000068035%2C000067788%2C000067528%2C000067981%2C000067947%2C000067483%2C000068094%2C000067893%2C000068151%2C000067784%2C000068024%2C000067590%2C000067811%2C000067971%2C000069381%2C000067484%2C000067824%2C000067493%2C000067912%2C000067588%2C000067589%2C000067515%2C000067781%2C000067896%2C000067975%2C000067777%2C000067851%2C000068042%2C000067726%2C000067570%2C000067591%2C000067587%2C000068090%2C000068135%2C000068077%2C000067808%2C000068025%2C000067481%2C000069377%2C000068154%2C000067747%2C000067592%2C000067510%2C000067684%2C000067783%2C000067968%2C000067782%2C000068136%2C000067862%2C000068074%2C000067482%2C000067919%2C000067619%2C000067513%2C000067921%2C000067929%2C000067976%2C000068020%2C000067807%2C000067864%2C000067906%2C000067745%2C000068064%2C000067741%2C000067765%2C000067509%2C000067586%2C000067890%2C000067472%2C000067957%2C000067948%2C000067746%2C000067889%2C000067479%2C000067796%2C000068038%2C000067547%2C000067966%2C000067922%2C000067967%2C000067812%2C000068026%2C000069380%2C000067742%2C000067994%2C000067999%2C000067585%2C000068012%2C000067516%2C000067511%2C000067965%2C000067728%2C000067884%2C000067970%2C000068039%2C000067860%2C000068096%2C000067495%2C000067520%2C000068128%2C000068147%2C000068152%2C000068043%2C000068007%2C000067908%2C000068070%2C000068115%2C000067867%2C000067958%2C000067743%2C000068076%2C000068086%2C000068091%2C000067883%2C000067752%2C000068000%2C000067480%2C000068148%2C000067885%2C000067546%2C000067762%2C000068063%2C000067639%2C000067920%2C000068062%2C000067868%2C000067660%2C000068237%2C000067569%2C000068238%2C000068155%2C000067512%2C000068131%2C000067866%2C000067979%2C000067794%2C000068169%2C000068058%2C000067640%2C000068005&blanks=BLANK1.1A%2CBLANK1.1B%2CBLANK1.1C%2CBLANK1.1D%2CBLANK1.1E%2CBLANK1.1F%2CBLANK1.1G%2CBLANK1.1H%2CBLANK2.2A%2CBLANK2.2B%2CBLANK2.2C%2CBLANK2.2D%2CBLANK2.2E%2CBLANK2.2F%2CBLANK2.2G%2CBLANK2.2H%2CBLANK3.3A%2CBLANK3.3B%2CBLANK3.3C%2CBLANK3.3D%2CBLANK3.3E%2CBLANK3.3F%2CBLANK3.3G%2CBLANK3.3H%2CBLANK4.4A%2CBLANK4.4B%2CBLANK4.4C%2CBLANK4.4D%2CBLANK4.4E%2CBLANK4.4F%2CBLANK4.4G%2CBLANK4.4H%2CBLANK5.5A%2CBLANK5.5B%2CBLANK5.5C%2CBLANK5.5D%2CBLANK5.5E%2CBLANK5.5F%2CBLANK5.5G%2CBLANK5.5H%2CBLANK6.6A%2CBLANK6.6B%2CBLANK6.6C%2CBLANK6.6D%2CBLANK6.6E%2CBLANK6.6F%2CBLANK6.6G%2CBLANK6.6H&external=&selected_ag_surveys=-1%2C-2%2C-3%2C-4%2C-5&merged=False |
"Is there a nicer way to decode utf-8 here?" >>> s = 'é'
>>> type(s)
<type 'str'>
>>> s.decode('utf-8')
u'\xe9' ...python27 is really bad with unicode unfortunately. super annoying |
LEFT JOIN ag.source_barcodes_surveys USING (barcode) | ||
LEFT JOIN ag.ag_login_surveys USING (survey_id) | ||
LEFT JOIN ag.ag_login | ||
ON (ag.ag_kit.ag_login_id = ag.ag_login.ag_login_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USING (ag_login_id)
doesn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, because there are three tables holding ag_login_id: ag_kit, ag_login_surveys and ag_login
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
|
In [7]: e.message.decode('utf-8')
Out[7]: u'\xe9' You need to access to the message attribute of the error. |
@sjanssen2, can you try changing the offending line (from the traceback in the thread):
to:
It may just be a matter of the wrong method off of |
I changed to |
This is now good for reviews @wasade @josenavas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment
@@ -962,7 +970,10 @@ def _decode_zip_lookup(item): | |||
'SURVEY_ID'], unknown_external)) | |||
except Exception as e: | |||
# Add barcode to error and remove from metadata info | |||
errors[barcode] = '%s' % e | |||
if isinstance(e, SSLError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised about this - SSLError should be a subclass of exception, so it should have the message
attribute. Why the special handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it doesn't. I was surprised too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, it does work on my machine:
In [5]: from requests.exceptions import SSLError
In [6]: try:
...: raise SSLError()
...: except Exception as e:
...: pass
...:
In [7]: e.message
Out[7]: ''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it works on my laptop and on Travis, but not on flight. I figure it is due to different lib versions. And to be more precise, it seems to be the "message" object itself which does not work as expected if the string is utf-8. Other Exceptions work as expected, but not SSLError.
I'd be happy if we could defer a cleaner solution and continue working on the pulldown.
I'm 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments.
JOIN ag.ag_kit_barcodes USING (ag_kit_id) | ||
WHERE supplied_kit_id = %s""" | ||
barcodes = db._con.execute_fetchall(sql, [supplied_kit_id]) | ||
if barcodes != []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace by if barcodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
WHERE supplied_kit_id = %s AND barcode NOT IN %s""" | ||
barcodes = db._con.execute_fetchall(sql, [supplied_kit_id, | ||
tuple(old_barcodes)]) | ||
if barcodes != []: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
knimin/tests/test_ag_edit_barcode.py
Outdated
LEFT JOIN ag.ag_login_surveys USING (ag_login_id) | ||
WHERE barcode = %s""" | ||
sourcenames = db._con.execute_fetchall(sql, [barcode]) | ||
self.assertTrue(sourcenames is not None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertIsNotNone(sourcenames)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
knimin/tests/test_ag_edit_barcode.py
Outdated
response = self.post('/ag_edit_barcode/', payload) | ||
|
||
self.assertEqual(response.code, 200) | ||
self.assertEqual(new_sourcename, obs_details['participant_name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to self.assertEqual(obs_details['participant_name'], new_sourcename)
It is expected that the first parameter is the observed value and the second one is the expected one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -8,6 +8,10 @@ | |||
|
|||
|
|||
class TestAGEditBarcodeHandler(TestHandlerBase): | |||
# these fields switch None representation and thus are hard to check | |||
# for equality | |||
none_fields = ['environment_sampled', 'withdrawn', 'refunded'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean that they change None representation? If they go from some value to None and that's the expected behavior you can check that they're actually None
, otherwise they go untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had too much time to investigate this, but I saw that different methods represent the same field differently. Sometimes it's 'N', or None or '' or 'None'.
Since it did not affect my tests here, I did not want to spend more time with it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
knimin/tests/test_ag_edit_barcode.py
Outdated
# check that no actual change has happened | ||
|
||
dbinfo = db.getAGBarcodeDetails(barcode) | ||
for field in set(payload.keys()) - set(self.none_fields): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a copy of payload in which you change this values to None, so you actually test that those values are None as you expect, rather than not testing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about this approach:
if field in self.none_fields:
if details[field] in [None, 'N', 'None', '']:
details[field] = None
if dbinfo[field] in [None, 'N', 'None', '']:
dbinfo[field] = None
self.assertEqual(details[field], dbinfo[field])```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more concerned about the fact that we don't know which value is going to be returned. That can be fatal for assumptions done in the interface using the function db.getAGBarcodeDetails
. Specially I'm surprised that the function db.getAGBarcodeDetails is not consistent in the way that it reports values...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% agree that these are serious concerns, but can we please keep this out of the scope of this PR?
knimin/tests/test_ag_edit_barcode.py
Outdated
response = self.post('/ag_edit_barcode/', payload) | ||
|
||
self.assertEqual(response.code, 200) | ||
self.assertEqual(None, obs_details['participant_name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertIsNone(obs_details['participant_name'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
knimin/tests/test_ag_edit_barcode.py
Outdated
|
||
self.assertEqual(response.code, 200) | ||
self.assertEqual(None, obs_details['participant_name']) | ||
self.assertEqual(obs_surveys, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertIsNone(obs_surveys)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
knimin/tests/test_ag_edit_barcode.py
Outdated
self.assertEqual(response.code, 200) | ||
self.assertEqual(None, obs_details['participant_name']) | ||
self.assertEqual(obs_surveys, None) | ||
self.assertTrue(db.get_barcode_survey(barcode) is not None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertIsNotNone(db.get_barcode_survey(barcode))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -8,6 +8,10 @@ | |||
|
|||
|
|||
class TestAGEditBarcodeHandler(TestHandlerBase): | |||
# these fields switch None representation and thus are hard to check | |||
# for equality | |||
none_fields = ['environment_sampled', 'withdrawn', 'refunded'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@wasade can you check and merge if ok? |
Justine found: |
+1 on pets freetext change. @josenavas @wasade are we ok to merge? |
Code looks good to me - however I would hold merging until all functionality has been tested and we make sure that everything is working as expected. Does it make sense? |
Note: this PR will fail until DB is updated for amgut.I have adapted the changes to the DB in this PR for the integration of "source_barcodes_survey"