PubMedLabs update to PubMed.js #1801

akinhwan · Dec 19, 2018

updated getUID and getSearchResults functions, added a testCase for PubMedLabs

for the function getUID(), should I have logic for when user is on the "search results page" where there might be multiple UIDs? Or is this function purposefully meant for single UID at a time?

CheViana · Dec 20, 2018

CheViana reviewed Dec 20, 2018

View changes

PubMed.js

CheViana · Dec 20, 2018

CheViana reviewed Dec 20, 2018

View changes

PubMed.js

CheViana · Dec 20, 2018

CheViana reviewed Dec 20, 2018

View changes

PubMed.js

CheViana · Dec 20, 2018

CheViana reviewed Dec 20, 2018

View changes

PubMed.js

akinhwan · Dec 21, 2018

@zuphilip Hi Phillip, thanks for writing the PubMed.js. I'm trying to fix some user errors noticed on the new Pub Med Labs site. Namely, abstract pages are not being recognizes as "journalArticle" types. Are the commented out code in the doWeb function still needed?

adam3smith · Dec 21, 2018

Are the commented out code in the doWeb function still needed?

Almost certainly not, no. These have been there for a couple of years and no one has missed them. Feel free to remove during re-write.

akinhwan · Dec 26, 2018

All tests are passing in Scaffold!

akinhwan · Dec 31, 2018

@adam3smith could you take a look at this PR when you get a chance? I made sure all tests are passing in Scaffold. Let me know if there is anything you'd like me to address! Thanks

CheViana · Jan 2, 2019

Could somebody with write access to repo please merge this? @adam3smith @zuphilip

adam3smith · Jan 3, 2019

We'll need to review before merging, but as we indicated at @akinhwan ' s post above, are aware of this and will do asap.

adam3smith · Feb 3, 2019

@zuphilip -- do you have time to give this a quick look. I ran all automated tests without problems.

zuphilip · Feb 3, 2019

zuphilip reviewed Feb 3, 2019

View changes

This looks good. I have fixed two other warnings in the old code and added some comments. I have two more questions for you @adam3smith .

zuphilip · Feb 3, 2019

PubMed.js

-	if (!main) return;
-
+	var booksections = doc.getElementById('book-sections');
+	if (!main && !booksections) return;


I don't understand the logic here which you just changed recently. It is possible that main does not exist but booksections does, then this would continue (not both conditions fulfilled) and the next line would fail. I.e. main has to exist for the next line, but then we can change back to the original condition, because this will not change anything else. 🤔

Good catch. I'm not sure there are cases where just booksections appears, but in any case we're not using them, so I've removed booksections here entirely and make sure we don't expect it in detectWeb.

It might be that there are book sections without UID and for those this translator won't detect (or import) now -- if we find one in the future, we can simply add logic in this function.

Given that the parseFromItemprop function only covers books, I don't believe anyone has every come across an example.

zuphilip · Feb 3, 2019

PubMed.js

-			title: ZU.trimInternal(title),
-			checked: checkbox && checkbox.checked
-		};	
+	if (results.length > 0){


Isn't it enough to either do the test on line 146 and fail when length is zero or do the condition here such that the following lines are only called when the length is positive?

zuphilip · Feb 4, 2019

🎉 Thank you very much @akinhwan @CheViana @adam3smith for working on this collaboratively! This is now squashed and merged and thereby will appear for the Zotero's users soon.

akinhwan · Feb 12, 2019

@zuphilip @adam3smith thank You so how would I know when it is live? Would it be in the next release? https://www.zotero.org/support/changelog Thanks :)

adam3smith · Feb 12, 2019

Translators update daily, independent of releases, so this has been live for >a week.

Kim added some commits Dec 19, 2018

updated getUID and getSearchResults functions, added a testCase for P…
…ubMedLabs

e622323

added logical OR for getUID

Loading status checks…

e72a1a5

Kim added some commits Dec 20, 2018

updated getSearchResults function, and regex target

Loading status checks…

813ddfb

added test case for search results page with multiple docsums

Loading status checks…

8b34583

updated getUID labs_uid variable

Loading status checks…

997d59a

akinhwan changed the title ~~[WIP] PubMedLabs update to PubMed.js~~ PubMedLabs update to PubMed.js Dec 24, 2018

detectWeb bookSections, getSearchResults for multiple docsums, more l…
…abs testCases

Loading status checks…

af7685a

akinhwan referenced this pull request Jan 15, 2019
Closed
Zotero Item Selector unchecked selections #1804

Fix two other warnings of the linter
The escape function is deprecated and therefore use decodeURIComponent instead.

Loading status checks…

d17d19a

No need to check results.length twice

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

b0c19b8

Finetune itemprop for books

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

7b84f57

zuphilip merged commit 5772369 into zotero:master Feb 4, 2019
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zotero/translators

Join GitHub today

PubMedLabs update to PubMed.js #1801

Conversation

akinhwan commented Dec 19, 2018 • edited

Kim added some commits Dec 19, 2018

CheViana reviewed Dec 20, 2018 View changes

CheViana reviewed Dec 20, 2018 View changes

CheViana reviewed Dec 20, 2018 View changes

CheViana reviewed Dec 20, 2018 View changes

Kim added some commits Dec 20, 2018

This comment has been minimized.

akinhwan commented Dec 21, 2018 • edited

This comment has been minimized.

adam3smith commented Dec 21, 2018

akinhwan changed the title [WIP] PubMedLabs update to PubMed.js PubMedLabs update to PubMed.js Dec 24, 2018

This comment has been minimized.

akinhwan commented Dec 26, 2018 • edited

This comment has been minimized.

akinhwan commented Dec 31, 2018

akinhwan and others added some commits Jan 2, 2019

This comment has been minimized.

CheViana commented Jan 2, 2019

This comment has been minimized.

adam3smith commented Jan 3, 2019

akinhwan referenced this pull request Jan 15, 2019

Zotero Item Selector unchecked selections #1804

adam3smith added some commits Feb 2, 2019

This comment has been minimized.

adam3smith commented Feb 3, 2019

zuphilip reviewed Feb 3, 2019 View changes

zuphilip left a comment

This comment has been minimized.

zuphilip Feb 3, 2019

This comment has been minimized.

adam3smith Feb 4, 2019

This comment has been minimized.

zuphilip Feb 3, 2019 • edited

Hide details View details zuphilip merged commit 5772369 into zotero:master Feb 4, 2019 1 check passed

1 check passed

This comment has been minimized.

zuphilip commented Feb 4, 2019

This comment has been minimized.

akinhwan commented Feb 12, 2019

This comment has been minimized.

adam3smith commented Feb 12, 2019

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

akinhwan commented Dec 19, 2018 •

edited

CheViana reviewed Dec 20, 2018

View changes

CheViana reviewed Dec 20, 2018

View changes

CheViana reviewed Dec 20, 2018

View changes

CheViana reviewed Dec 20, 2018

View changes

akinhwan commented Dec 21, 2018 •

edited

akinhwan commented Dec 26, 2018 •

edited

zuphilip reviewed Feb 3, 2019

View changes

zuphilip Feb 3, 2019 •

edited

zuphilip merged commit `5772369` into zotero:master Feb 4, 2019
1 check passed