Skip to content

/translators

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.

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

[Zenodo] Prevent mis-importing software as report #1578

Merged
merged 10 commits into from Mar 9, 2018
+71 −15

Conversation

Reviewers

@adam3smith adam3smith

@zuphilip zuphilip

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@katrinleinweber @adam3smith @zuphilip
@katrinleinweber
Copy link
Contributor

katrinleinweber commented Mar 7, 2018 •
edited

As discussed in the forum this ensures that Software items on Zenodo are imported as computerProgram, even though along the way, Zenodo is offering a non-CSL item type

Potential/Remaining isues to discuss

  • Scaffold imports the full author names as lastName. This could occur in https://github.com/zotero/translators/blame/master/Zenodo.js#L172-L180, but I'm not sure whether or how to adjust that. Or, should I adjust the new test case to expect lastName and firstName normally?
  • Would it be helpful, if Zenodo stopped putting a clearly mis-matching CSL type in the JSON that Zotero parses, and instead send none? If yes, could we then omit both the "fix" and this "undo"?
  • Scaffold added - and + in the doWeb test output (near the bottom in the diff). Not sure what those mean, but I guess both symbols need to be removed from the JSON block.

katrinleinweber added some commits Mar 7, 2018

@katrinleinweber
Prevent mis-importing software as report
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…
6d94ac5
@katrinleinweber
Remove symbols that Scaffold added to doWeb test output
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…
7ebc987
@katrinleinweber
Fix indentation
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…
b2d7d61
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Collaborator

adam3smith commented Mar 7, 2018

How are you generating the tests in Scaffold? This is fine now, but to save you the work going forward, if you actually generate the test using the Scaffold testing panel, you won't get the + and - (which are from a diff display Scaffold shows).
@adam3smith

adam3smith reviewed Mar 7, 2018
View changes

Zenodo.js Outdated
@@ -195,6 +195,11 @@ function scrape(doc, url) {

if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];

This comment has been minimized.

Sign in to view
Copy link
@adam3smith

adam3smith Mar 7, 2018

Collaborator

what would happen if we replace this whole thing, including l. 197 by item.itemType = detectWeb(doc, url)
That seems like a cleaner solution, provided our detect function is good.

This comment has been minimized.

Sign in to view
Copy link
@katrinleinweber

katrinleinweber Mar 7, 2018

Author Contributor

Could the var zoteroType block be removed as well, then?

This comment has been minimized.

Sign in to view
Copy link
@adam3smith

adam3smith Mar 7, 2018

Collaborator

exactly, yes
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Collaborator

adam3smith commented Mar 7, 2018

The Zenodo item types in the CSL are fine -- they are as good as it gets for e.g. automated citation generation directly from the CSL and Zotero can customize at will in the import.

For the names, we can't really parse these correctly without breaking institutional names since they're entered incorrectly in Zenodo, but we can switch fieldMode to true along the lines for

for (let i =0; i< item.creators.length; i++) {
  if (!item.creators[i].firstName) {
    item.creators[i].fieldMode = true;
  }
}

This makes them easier to fix manually.

katrinleinweber added some commits Mar 7, 2018

@katrinleinweber
Implement reviewer's suggestion to rely only on detectWeb 
https://twitter.com/thepracticaldev/status/725346935759056897
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…
d255ec5
@katrinleinweber
Support manually fixing mashed lastNames
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…
2b436be
@zuphilip

zuphilip reviewed Mar 7, 2018
View changes

Zenodo.js Outdated
@@ -175,6 +175,7 @@ function scrape(doc, url) {
if (!item.creators[i].firstName && item.creators[i].lastName.indexOf(",")!=-1) {
item.creators[i].firstName = item.creators[i].lastName.replace(/.+?,\s*/, "");
item.creators[i].lastName = item.creators[i].lastName.replace(/,.+/, "");
item.creators[i].fieldMode = true;
}

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Mar 7, 2018

Collaborator

Does this work? No changes in the test cases??

I think what we want is rather want is:

if (!item.creators[i].firstName) {
	if (item.creators[i].lastName.indexOf(",")!=-1) {
		item.creators[i].firstName = item.creators[i].lastName.replace(/.+?,\s*/, "");
		item.creators[i].lastName = item.creators[i].lastName.replace(/,.+/, "");
	} else {
		item.creators[i].fieldMode = true;
	}
}
Zenodo.js

if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];
}

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Mar 7, 2018

Collaborator

Can we delete the first introduction of the variable type as well?

This comment has been minimized.

Sign in to view
Copy link
@katrinleinweber

katrinleinweber Mar 8, 2018

Author Contributor

Do you mean L42 or L133? Removing the latter indeed doesn't break the tests :-)

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Mar 8, 2018

Collaborator

Yes, the latter one seems not necessarily anymore. We certainly still need the first one.
Zenodo.js Outdated
if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];
}
item.itemType = detectWeb(doc, url)

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Mar 7, 2018

Collaborator

Add ; at the end.
Zenodo.js Outdated
"lastName": "Carl Boettiger"
"firstName": ""
"creatorType": "author"
}

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Mar 7, 2018

Collaborator

Rerun the tests should yield some fieldMode fields.

@zuphilip zuphilip merged commit 5fa9a68 into zotero:master Mar 9, 2018
1 check passed

1 check passed

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

zuphilip commented Mar 9, 2018

🎉 Thank you @katrinleinweber !
@katrinleinweber

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

katrinleinweber commented Mar 9, 2018

You're welcome, and thanks for merging :-)

This was referenced Mar 9, 2018

Open
Open

@katrinleinweber katrinleinweber referenced this pull request Jan 24, 2019

Open

collaboration with Zotero #91

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.