Conversation
kelockhart
left a comment
kelockhart
left a comment
There was a problem hiding this comment.
Mostly small stuff, but one more test would be good to make sure we can handle records without bibcodes. Plus a clarifying question on the source renaming.
README.md
Outdated
| curl http://localhost:4000/<identifier> | ||
|
|
||
| and you should get back graphics data | ||
| and you should get back graphics data. The identifier supplied should either be a bibcode or a SciX ID. |
There was a problem hiding this comment.
Consider adding an example of a SciX ID - I imagine (at least at first) some confusion over whether to include the scix: part of the identifier
| res = {'Error': 'Unable to get results!', 'Error Info': 'No database entry found for %s' % identifier} | ||
| except Exception as err: | ||
| res = {'Error': 'Unable to get results!', 'Error Info': 'Graphics query failed for %s: %s'%(bibcode, err)} | ||
| res = {'Error': 'Unable to get results!', 'Error Info': 'Graphics query failed for %s: %s'%(identifier, err)} |
There was a problem hiding this comment.
Add explicit handling for MultipleResultsFound as well, just in case
graphics_service/graphics.py
Outdated
| output['scix_id'] = results['scix_id'] | ||
| # We still include the value in the "bibcode" column in the output | ||
| # For backwards compatibility | ||
| output['bibcode'] = results['bibcode'] |
There was a problem hiding this comment.
It'll be possible in the near future to have a record with a SciX ID but no bibcode - probably best to do a results.get('bibcode') (which will return None if it needs to) vs results['bibcode'] which would give a KeyError if there's no bibcode for that record
| @mock.patch('graphics_service.models.execute_SQL_query', return_value=get_testdata(figures=figure_data)) | ||
| def test_query(self, mock_execute_SQL_query): | ||
| def test_query_with_scixid(self, mock_execute_SQL_query): | ||
| '''Query endpoint with bibcode from stub data should |
There was a problem hiding this comment.
Copy/paste error here - bibcode --> SciX ID
There was a problem hiding this comment.
It'd be good to add one test where there's a GraphicsModel with a SciX ID but no bibcode, just to make sure that part works
| 'OUP':'Every image links to the article on <a href="https://academic.oup.com/mnras/" target="_new">Monthly Notices of the RAS</a>', | ||
| 'IOP':'Every image links to the <a href="http://www.astroexplorer.org/" target="_new">AAS "Astronomy Image Explorer"</a> for more detail.', | ||
| 'IOPscience':'Every image links to the article on <a href="http://iopscience.iop.org/" target="_new">IOPscience</a>', | ||
| 'AAS':'Every image links to the <a href="http://www.astroexplorer.org/" target="_new">AAS "Astronomy Image Explorer"</a> for more detail.', |
There was a problem hiding this comment.
Does this renaming mean we need to update whatever is currently stored in the graphics db to match the new naming scheme?
There was a problem hiding this comment.
The only thing that changes in the graphics db is the addition of the column for the SciX ID. Everything else remains unchanged.
3 participants
This update update of the
graphics_serviceaccomplishes the following:The query
will fail if the
scix_idcolumn does not exsist. Since this column will be added with this new release, it seems unnecessary to require the code to be able to run against a database with the old schema.After the new release has been deployed, the database with be provisioned with appropriate SciX ID values.