Skip to content

Improved error handling and added cover image functionality #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SarahZum merged 3 commits into from
Jul 27, 2017
Merged

Improved error handling and added cover image functionality #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14a27c5
Update browzineJournals.js
SarahZum Jul 27, 2017
2f612f3
Update browzineArticleInContext,js
SarahZum Jul 27, 2017
8700458
Update browzine.js
SarahZum Jul 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 51 additions & 33 deletions js/browzine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,44 +7,62 @@
$sceDelegateProvider.resourceUrlWhitelist(urlWhitelist);
}]);

// Add Browzine Links
app.controller('prmBriefResultAfterController', function($scope, $http) {
// Add Article In Context & Browzine Links
app.controller('prmSearchResultAvailabilityLineAfterController', function($scope, $http) {
var vm = this;
if (vm.parentCtrl.item.pnx.addata.doi) {
vm.doi = vm.parentCtrl.item.pnx.addata.doi[0] || '';
var url = "https://yourserver.edu/primo/browzine/browzineArticleInContext?DOI=" + vm.doi;
$http.jsonp(url, {jsonpCallbackParam: 'callback'}).then(function(response) {
$scope.article = response.data;
}, function(error){
console.log(error); //
if (vm.parentCtrl.result.pnx.addata.doi && vm.parentCtrl.result.pnx.display.type[0] == 'article') {
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊 🚲

Copy link
Copy Markdown
Owner Author

@SarahZum SarahZum Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all, great tip--I'll give it a try!

vm.doi = vm.parentCtrl.result.pnx.addata.doi[0] || '';
var articleURL = "https://yourserver.edu/primo/browzine/browzineArticleInContext?DOI=" + vm.doi;
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊

Copy link
Copy Markdown
Owner Author

@SarahZum SarahZum Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readme updated! Let me know what could still use clarification??

Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$http.jsonp(articleURL, {jsonpCallbackParam: 'callback'}).then(function(response) {
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner Author

@SarahZum SarahZum Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, exactly--to get around all the domain-crossing going on :-)

$scope.article = response.data;
}, function(error){
console.log(error);
});
}

}
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;
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Owner Author

@SarahZum SarahZum Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call--and I actually already did set the constant when whitelisting our server address so I should utilize it!

$http.jsonp(journalURL, {jsonpCallbackParam: 'callback'}).then(function(response) {
$scope.journal = response.data;
}, function(error){
console.log(error);
});
}

//TO DO: add control so both links don't show up for a single record...
});

app.component('prmSearchResultAvailabilityLineAfter', {
bindings: { parentCtrl: '<' },
controller: 'prmSearchResultAvailabilityLineAfterController',
template: `
<div ng-if="article.data.browzineWebLink"><a href="{{ article.data.browzineWebLink }}" target="_blank"> See article in Table of Contents!</a></div>
<div ng-if="journal.data[0].browzineWebLink"><a href="{{ journal.data[0].browzineWebLink }}" target="_blank"> Browse this journal in Browzine!</a></div>
`
});

if (vm.parentCtrl.item.pnx.addata.issn) {
// Add Journal Cover Images from Browzine
app.controller('prmSearchResultThumbnailContainerAfterController', function($scope, $http) {
var vm = this;
var newThumbnail = '';
if (vm.parentCtrl.item.pnx.addata.issn) {
vm.issn = vm.parentCtrl.item.pnx.addata.issn[0].replace("-", "") || '';
var url2 = "https://yourserver.edu/primo/browzine/browzineJournals?ISSN=" + vm.issn;
$http.jsonp(url2, {jsonpCallbackParam: 'callback'}).then(function(response) {
$scope.journal = response.data;
console.log(response.data);
var journalURL = "https://yourserver.edu/primo/browzine/browzineJournals?ISSN=" + vm.issn;
$http.jsonp(journalURL, {jsonpCallbackParam: 'callback'}).then(function(response) {
newThumbnail = response.data.data["0"].coverImageUrl;
}, function(error){
console.log(error); //
});
}

});
console.log(error); //
});
}
vm.$doCheck = function(changes) {
if (vm.parentCtrl.selectedThumbnailLink) {
if (newThumbnail != '' && (vm.parentCtrl.selectedThumbnailLink.linkURL.indexOf("icon_journal.png") != -1 || vm.parentCtrl.selectedThumbnailLink.linkURL.indexOf("img/icon_article.png") != -1) ) {
vm.parentCtrl.selectedThumbnailLink.linkURL = newThumbnail;
}
}
};
});

app.component('prmBriefResultAfter', {
bindings: { parentCtrl: '<' },
controller: 'prmBriefResultAfterController',
template: `

<div ng-if="article.data.browzineWebLink"><blockquote><a href="{{ article.data.browzineWebLink }}" target="_blank">See in Table of Contents!</a> ({{$ctrl.doi}}) </blockquote></div>
<div ng-if="journal.data[0].browzineWebLink"><blockquote><a href="{{ journal.data[0].browzineWebLink }}" target="_blank">Browse this journal in Browzine!</a> ({{$ctrl.issn}}) </blockquote></div>


`
});
app.component('prmSearchResultThumbnailContainerAfter', {
bindings: { parentCtrl: '<' },
controller: 'prmSearchResultThumbnailContainerAfterController',
});
28 changes: 21 additions & 7 deletions nodejs/browzineArticleInContext,js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,33 @@ exports.ArticleLookup = function (event, context, callback) {
var stoID = process.env['stoID'];
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good job keeping the secrets out of the repo 😊

var options = {
host : 'api.thirdiron.com',
path : '/public/v1/libraries/' + stoID + '/articles/doi/' + DOI + "?access_token=" + stoKey,
port : 443,
path : '/public/v1/libraries/' + stoID + '/articles/doi/' + DOI + '?access_token=' + stoKey,
method : 'GET'
};

//make the https get call to Browzine and pass the callback data to Primo
// make the https get call to Browzine and pass the callback data to Primo
var getReq = https.request(options, function(res) {
res.on('data', function(data) {
callback(null,
cb + '(' + data + ')'
);

var data = '';
res.on('data', function(chunk) {
data += chunk.toString();
});

res.on('end', function(){
if (res.statusCode == 200) {
callback(null,
cb + '(' + data + ')'
);
} 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":[]})'
Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - though if we put in an empty object with our 404, would that be nicer?

Copy link
Copy Markdown
Owner Author

@SarahZum SarahZum Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor

@karlbecker karlbecker Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - we'll stick with it for now, but I'm making a note for a future potential change.

);
}
});
});

//end the request
getReq.end();
getReq.on('error', function(err){
Expand Down
27 changes: 20 additions & 7 deletions nodejs/browzineJournals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,33 @@ exports.JournalLookup = function (event, context, callback) {
var stoID = process.env['stoID'];
var options = {
host : 'api.thirdiron.com',
port : 443,
path : '/public/v1/libraries/' + stoID + '/search?issns=' + ISSN + '&access_token=' + stoKey,
method : 'GET'
};

//make the https get call to Browzine and pass the callback data to Primo
// make the https get call to Browzine and pass the callback data to Primo
var getReq = https.request(options, function(res) {
res.on('data', function(data) {
callback(null,
cb + '(' + data + ')'
);
var data = '';
res.on('data', function(chunk) {
data += chunk.toString();
});
});

res.on('end', function(){
if (res.statusCode == 200) {
callback(null,
cb + '(' + data + ')'
);
} 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":[]})'
);
}
});
});

//end the request
// end the request
getReq.end();
getReq.on('error', function(err){
console.log("Error: ", err);
Expand Down