Improved error handling and added cover image functionality#1
Improved error handling and added cover image functionality#1
Conversation
Refinements to error handling
Added error handling
Additional controls for display of browse links and added cover images
karlbecker
left a comment
karlbecker
left a comment
There was a problem hiding this comment.
Looks good overall!
One question: have you considered writing any kind of automated acceptance tests, even if they're just against the node app?
If you'd like some guidance, I could try doing it - although I would introduce npm into the repo then, probably using mocha to make a little basic automated test.
Just a thought!
| @@ -13,19 +13,33 @@ exports.ArticleLookup = function (event, context, callback) { | |||
| var stoID = process.env['stoID']; | |||
There was a problem hiding this comment.
👍 good job keeping the secrets out of the repo 😊
| $scope.article = response.data; | ||
| }, function(error){ | ||
| console.log(error); // | ||
| if (vm.parentCtrl.result.pnx.addata.doi && vm.parentCtrl.result.pnx.display.type[0] == 'article') { |
There was a problem hiding this comment.
Little formatting nit here, but the (probably unintentional?) whitespace changes here can tend to mess up diffs and make reviews a little longer to get through.
I thought I'd mention something that's been useful on our team: the .editorconfig file, as described at http://editorconfig.org
You put it in the root directory of your project, and when opening that directory in a text editor, many text editors recognize the preferences there and will respect them, independent of their own preferred settings. It'll help changes like this here - swapping in tabs for spaces - from unintentionally happening.
Hopefully this wasn't too much of a bike shed comment 😊 🚲
There was a problem hiding this comment.
Not at all, great tip--I'll give it a try!
| console.log(error); // | ||
| if (vm.parentCtrl.result.pnx.addata.doi && vm.parentCtrl.result.pnx.display.type[0] == 'article') { | ||
| vm.doi = vm.parentCtrl.result.pnx.addata.doi[0] || ''; | ||
| var articleURL = "https://yourserver.edu/primo/browzine/browzineArticleInContext?DOI=" + vm.doi; |
There was a problem hiding this comment.
So the guidance for first-time setup would be to simply change yourserver.edu to wherever we're installing things, correct?
Also, is the js/browzine.js file something that each Primo user must put somewhere into the Primo installation?
I'm sure more full instructions are coming, so I'll stop my questions about setup instructions until I see them 😊
There was a problem hiding this comment.
readme updated! Let me know what could still use clarification??
There was a problem hiding this comment.
Thanks Sarah! Readme answers those questions now, excellent.
I will try setting something up with Amazon Lambda, because that sounds like a great use for it and would make setup much simpler than setting up a variety of EC2 boxes.
| } | ||
| if (vm.parentCtrl.result.pnx.addata.issn && vm.parentCtrl.result.pnx.display.type[0] == 'journal') { | ||
| vm.issn = vm.parentCtrl.result.pnx.addata.issn[0].replace("-", "") || ''; | ||
| var journalURL = "https://yourserver.edu/primo/browzine/browzineJournals?ISSN=" + vm.issn; |
There was a problem hiding this comment.
Since yourserver.edu is sprinkled throughout this file, would it make sense to define it once up at the top, then use string literals throughout the rest of the file, to reduce the likelihood of typos?
Something like:
const serverName = 'yourserver.edu';
...
var journalURL = `https://${serverName}/primo/browzine/browzineJournals?ISSN=${vm.issn}`;
perhaps? Changing yourserver.edu becomes a one-line change instead of a multi-line find and replace in this case.
There was a problem hiding this comment.
Good call--and I actually already did set the constant when whitelisting our server address so I should utilize it!
| } else { | ||
| // Browzine's API returns a 404 if data isn't found, so send an empty response if call is unsuccessful | ||
| callback(null, | ||
| cb + '({"data":[]})' |
There was a problem hiding this comment.
Makes sense - though if we put in an empty object with our 404, would that be nicer?
There was a problem hiding this comment.
Would save a little bit on the conditional logic, but I understand your perspective on returning a 404 for a bad DOI (I didn't realize that was what was going on with those queries), so I'm content either way!
There was a problem hiding this comment.
Cool - we'll stick with it for now, but I'm making a note for a future potential change.
| if (vm.parentCtrl.result.pnx.addata.doi && vm.parentCtrl.result.pnx.display.type[0] == 'article') { | ||
| vm.doi = vm.parentCtrl.result.pnx.addata.doi[0] || ''; | ||
| var articleURL = "https://yourserver.edu/primo/browzine/browzineArticleInContext?DOI=" + vm.doi; | ||
| $http.jsonp(articleURL, {jsonpCallbackParam: 'callback'}).then(function(response) { |
There was a problem hiding this comment.
I haven't used jsonp before, though I've ready about it - will need to read a little bit to remind myself precisely how it works! Seems to make sense here though: a string is returned back from the node server to here, and the jsonp function decodes it and builds json from the response payload, correct?
There was a problem hiding this comment.
Yep, exactly--to get around all the domain-crossing going on :-)
2 participants
No description provided.