Upgrade to ORCiD 2.0 #1269

thostetler · 2017-10-12T09:07:50Z

Work in progress
Functionally complete, at least afaik, I need testers to check things out and make sure that everything is working like it did in prod, and whether there are areas to improve.

This is a big update, most of the changes are in Orcid_api and Extension

The orcid api actions portion was completely rewritten
- The getProfile and addWork actions are now debounced to help with number of requests
- Clean up and better handling of promises throughout, definitely still areas to improve.
extension
- Actions portion was completely rewritten
- Could be optimized more, I think there is information I could gather without making another call.
- Better error handling, properly rejecting promises. Hopefully more helpful feedback
  - Have to think about any better ways of providing this feedback to the user, I don't think we want to show a modal with some error message, for example, unless it is helpful in some way to the user.
Work/Profile
- Added some classes to help with the parsing/transforming of orcid/ads records
- Used jsonpath to get/set values using xpath-like paths, which should make maintenance easier in the future.
Thoughts
- Do we need to have a database for the api? I think that it really makes more sense when we had to pass the entire profile back to orcid.
  - Could potentially hold a database of items and cache changes, updating the ui to the internal state and pushing up changes after a wait.
- Edge cases
  - I found a few areas where there were bugs, I thought I had introduced, only to see them show up in prod. I think in those cases, they are mostly taken care of.
  - I am not sure how orcid handles the case of multiple sources in 2.0, but I was unclear how to update the source_name part for now, until I can find a use case for the changes.

* Major rewrite of logic for api * Updated to work with ORCiD 2.0 api * Offloaded all logic dealing with parsing/formatting to new Work and Profile classes

* Adds helper classes that deal with the parsing and formatting of ORCiD works * Hopefully easier to maintain, since all entries are now xpath-based

* Converts the extension over to the new api * Rewrite of handlers for orcid actions * Alters some of the logic dealing with updating the ui

*Work In Progress* * Some tests disabled/commented while they are being converted * Most tests are working * Added simple tests for new classes

romanchyla

i really like your PR, great work!

i've found few issues (of little significance); the only important one would be to udpate the unittests - orcid is quite tricky and before we can merge it should be well tested (for example, the api limits of 5000 characters - it used to be orcid throwing error; maybe it is not the case any more - but stuff like that is important)

romanchyla · 2017-10-13T16:39:20Z

test/mocha/js/modules/orcid/orcid_extension.spec.js


+        get(0, 'update').click();
+        get(1, 'delete').click();
+        get(2, 'add').click();


github shows me not enough context; but it seems like you concentrated 3 actions into one spot; the idea was to click 'update' and verify, then click 'delete' and verify....

yeah those need to be written out better, the three button clicks are different actions, but right after that it checks to see how many times the merge records method was called (should be once). There is a lot more that needs to be checked here though, I'd like to test for fails as well.

romanchyla · 2017-10-13T16:43:02Z

test/mocha/js/modules/orcid/profile.spec.js

+
+    describe('Instantiation', function () {
+      it('instantiates correctly', function () {
+        expect(new Profile() instanceof Profile).to.equal(true);


this suite should be extended

romanchyla · 2017-10-13T16:43:18Z

test/mocha/js/modules/orcid/work.spec.js

+
+    describe('Instantiation', function () {
+      it('instantiates correctly', function () {
+        expect(new Work({}) instanceof Work).to.equal(true);


dtto - but i guess that's your plan

yeah, I will add in these tests.

romanchyla · 2017-10-13T16:50:15Z

src/js/modules/orcid/orcid_api.js

-            }
-          });
-        return ret.promise();
+      checkAccessOrcidApiAccess: function() {


one significan tchange i see here is that authData is missing; i don't recall - whether it was ever needed (to pass a different token), but i'm assuming it was there for a reason (could be wrong)

romanchyla · 2017-10-13T17:01:59Z

src/js/modules/orcid/orcid_api.js

        }
-        return false;
+        return !!(this.authData);


pls double check authData always has '.expires'

romanchyla · 2017-10-13T17:32:44Z

src/js/modules/orcid/orcid_api.js

+        }
+
+        var url = this.getUrl('works', putcode);
+        return this.createRequest(url, { method: 'PUT' }, work);


i think it is better to remove promises (universally) for any methods that do add/update/delete work; here you are returning the request

Are you saying that the api shouldn't return anything on these requests? or should I be return more generic replies? like this:

var promise = $.Deferred(); request(...).done(function (status) { if (status === 201) { promise.resolve(); } else { promise.reject(); } }).fail(function (status) { promise.reject(); });

im sorry! i wanted to write: "better to return promises" universally

Ohh! okay yeah that makes sense. Actually in this case it does do that. createRequest is just a convenience wrapper around the call to sendData --> https://github.com/adsabs/bumblebee/pull/1269/files#diff-b88a44bee5c5ab37fb8be1b927cde460R332

There may be a cleaner way of doing it though, I think it makes sense to fail here for status codes that don't make sense than to force the caller to handle the response.

romanchyla · 2017-10-13T17:36:45Z

src/js/modules/orcid/orcid_api.js

-        this.getWorks()
-          .done(function (orcidWorks) {
+          _.forEach(works, function (w) {
+            var entry = self.addCache.pop();


question: is it guaranteed that the cache will have the same size/order or elements as the response from orcid? can multiple requests/user actions cause multiple requests?

romanchyla · 2017-10-13T17:45:05Z

src/js/modules/orcid/orcid_api.js

+      _buildQuery: function (query) {
+        var formatString = function (values, field) {
+          if (values.length === 0) {
+            return '';


that would be empty query -> syntax error; better not even send it to the api

romanchyla · 2017-10-13T17:52:02Z

src/js/modules/orcid/orcid_api.js

+          _.forEach(works, function addIdsToDatabase(w, i) {
+            var key;
+            var ids = w.getExternalIds();
+            if (ids.bibcode) {


solr now has a field called 'identifier' - it will search across multiple identifiers; so a query

bibcode:foo OR alternate_bibcode:foo OR doi:foo

can be replaced with:

identifier:foo

not sure why the old api didn't do that (maybe the identifier was not populated properly)

okay yeah that confused me a bit, I'll make that change it should make things a little easier

romanchyla · 2017-10-13T18:08:19Z

src/js/modules/orcid/extension.js

+          var works = profile.getWorks();
+          var matchedWork = _.find(works, function (w) {
+            var wIds = w.getExternalIds();
+            return exIds.bibcode === wIds.bibcode ||


bibcode can be only one; but doi and alternate_bibcodes are multivalued - so why check only the first element? ;)

* Add check for expires on authdata to ensure that it is present * Check access for orcid, now will also check if authdata is up to date * Add checks for empty query string to not send useless queries to solr * Remove use of `bibcode:xxx OR alternate_bibcode:xxx` queries for the `identifier:xxx` kind

* Better descriptions, jsdoc cleanup * Moved debounce calls into activation to help with testing

thostetler · 2017-10-20T20:34:18Z

@romanchyla this guy is ready for you to check out again.

I fixed, or altered the areas you mentioned. Some of the tests I've updated.
I was having tons of trouble with the sinon global getting dirtied as it moved through the tests, the state of things would break tests down the line. for future reference, I was able to eleviate most of that by wrapping all the test code in sinon.test(function () {...});

Also, I was able to figure out a good way to keep track of the right entries in the cache. I am passing along a unique id that rides the request, then I'm able to compare it afterwards to make sure it resolves the appropriate deferred.

I wasn't able to figure out a good way to gracefully fail after a bad addWorks request, yet... right now if you were to pass in say 300 at once, it would make 3 total requests, if request 2 failed then jquery short-circuits and calls the fail block right away. But if 1 and 2 succeeded we wouldn't know. Probably should refactor to handle that case, especially if the plan is to allow adding in bulk.

Thanks!

romanchyla · 2017-10-21T01:46:02Z

global variables are usually bad and most likely my ignorance of sinon framework made me use one for the tests. i'm glad it's gone :)

for addWorks failures; if some simple solution is not applicable - such as .done() and chaining of promises, then you could consider using sentinel values; maybe like adding an entry into the cache and removing it after the request finished; if you find some entry there, then you would know the batch failed

thostetler added 8 commits October 12, 2017 04:35

Add jsonpath library

b1b38b0

Update Orcid API

b38fbfc

* Major rewrite of logic for api * Updated to work with ORCiD 2.0 api * Offloaded all logic dealing with parsing/formatting to new Work and Profile classes

Add Work and Profile Classes

9ee1e60

* Adds helper classes that deal with the parsing and formatting of ORCiD works * Hopefully easier to maintain, since all entries are now xpath-based

Convert preferences widget

8952440

Convert orcid widget extension

528ae82

* Converts the extension over to the new api * Rewrite of handlers for orcid actions * Alters some of the logic dealing with updating the ui

Convert Orcid widget

5a1daa4

More conversions

044fe6e

Add/Update Tests

6863f56

*Work In Progress* * Some tests disabled/commented while they are being converted * Most tests are working * Added simple tests for new classes

thostetler added this to the Sprint 1 milestone Oct 13, 2017

romanchyla reviewed Oct 13, 2017

View reviewed changes

thostetler self-assigned this Oct 13, 2017

thostetler added 3 commits October 17, 2017 18:07

Improve Orcid Add Request Caching/Chunking

d7d00fb

Clean Up, small fixes

269a763

* Better descriptions, jsdoc cleanup * Moved debounce calls into activation to help with testing

thostetler force-pushed the upgrade-orcid-ui branch 2 times, most recently from e4cde0c to 208b14a Compare October 20, 2017 20:15

thostetler added 2 commits October 20, 2017 16:30

Fixes

4354fec

Update/Add Tests

79da3f8

thostetler force-pushed the upgrade-orcid-ui branch from 208b14a to 79da3f8 Compare October 20, 2017 20:30

thostetler force-pushed the upgrade-orcid-ui branch from d8955d9 to 79da3f8 Compare October 20, 2017 21:23

romanchyla merged commit 8cc6f79 into master Oct 21, 2017

thostetler deleted the upgrade-orcid-ui branch November 9, 2017 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to ORCiD 2.0 #1269

Upgrade to ORCiD 2.0 #1269

thostetler commented Oct 12, 2017

romanchyla left a comment

romanchyla Oct 13, 2017

thostetler Oct 13, 2017

romanchyla Oct 13, 2017

romanchyla Oct 13, 2017

thostetler Oct 13, 2017

romanchyla Oct 13, 2017

romanchyla Oct 13, 2017

romanchyla Oct 13, 2017

thostetler Oct 13, 2017 •

edited

Loading

romanchyla Oct 13, 2017

thostetler Oct 13, 2017

romanchyla Oct 13, 2017

romanchyla Oct 13, 2017

romanchyla Oct 13, 2017

thostetler Oct 13, 2017

romanchyla Oct 13, 2017

thostetler commented Oct 20, 2017 •

edited

Loading

romanchyla commented Oct 21, 2017

Upgrade to ORCiD 2.0 #1269

Upgrade to ORCiD 2.0 #1269

Conversation

thostetler commented Oct 12, 2017

romanchyla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thostetler Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thostetler commented Oct 20, 2017 • edited Loading

romanchyla commented Oct 21, 2017

thostetler Oct 13, 2017 •

edited

Loading

thostetler commented Oct 20, 2017 •

edited

Loading