Skip to content
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

Remove erroneous bit from codebook2-0.xsl (HTML Codebook stylesheet) #6963

Merged
merged 4 commits into from Jun 18, 2020

Conversation

johnhuck
Copy link
Contributor

@johnhuck johnhuck commented Jun 5, 2020

What this PR does / why we need it:
Removes an irrelevant hardcoded value ("ICPSR") in the HTML Codebook stylesheet.

Which issue(s) this PR closes:

Closes #6959

Special notes for your reviewer:
This updates my earlier pull request, based on discussion on the issue. Hope I did this right!

Suggestions on how to test this:

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
No
Additional documentation:

Replacing a hardcoded value with an XPath acquired value. I have made a presumption that the distributor name is the appropriate element to present here, since it is paired with the DOI value. This is my first Dataverse pull request, so let me know if I am missing a step or whatever.
Removing the XSLT that inserted the Distributor name at line 180.
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I didn't test this but the code change looks fine.

@pdurbin pdurbin moved this from Code Review 🦁 to QA 🔎✅ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Jun 5, 2020
@kcondon kcondon self-assigned this Jun 8, 2020
@kcondon
Copy link
Contributor

kcondon commented Jun 8, 2020

@pdurbin @johnhuck Seeing automated test failure on this branch, works in develop branch. It looks like the test is still looking for ICPSR in the comparison string. This is preventing me from building/deploying/testing:

[ERROR] Failures:
[ERROR] DdiExportUtilTest.testDatasetHtmlDDI:91 expected:<...

dct html ([ICPSR ]doi:10.5072/FK2/SOLY...> but was:<...

dct html ([]doi:10.5072/FK2/SOLY...>

@kcondon kcondon moved this from QA 🔎✅ to Code Review 🦁 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Jun 8, 2020
@kcondon kcondon removed their assignment Jun 8, 2020
@kcondon kcondon moved this from Code Review 🦁 to Community Dev 💻❤️ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Jun 8, 2020
@johnhuck
Copy link
Contributor Author

johnhuck commented Jun 8, 2020

thanks @kcondon and @pdurbin . I'm not sure what to do next. If the test is failing because it expects the string we want to remove, would it not be the case that the test needs to be modified?

@pdurbin
Copy link
Member

pdurbin commented Jun 8, 2020

@johnhuck that's exactly right. Based on the error above (line 91)...

Screen Shot 2020-06-08 at 12 39 11 PM

It looks like you should remove "ICPSR " from src/test/resources/html/dct_codebook.html

I can see it's in there with "grep":

$ grep ICPSR src/test/resources/html/dct_codebook.html
<h1>dct html&nbsp;(ICPSR doi:10.5072/FK2/SOLYMR)</h1>

@johnhuck
Copy link
Contributor Author

johnhuck commented Jun 8, 2020

@pdurbin OK. And this is something you want me to do as well and submit another PR? I can figure out how to do that. Just want to confirm that that's what you want me to do.

@pdurbin
Copy link
Member

pdurbin commented Jun 8, 2020

@johnhuck sure, please give it a shot. If you have any trouble, please make sure we can push to your branch so we can do it.

Removing "ICPSR" string so that the stylesheet change I made in IQSS#6963 will pass the automation test.
@coveralls
Copy link

coveralls commented Jun 8, 2020

Coverage Status

Coverage decreased (-0.03%) to 19.56% when pulling 9250585 on johnhuck:patch-1 into 97e7a60 on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Jun 8, 2020

I'm seeing https://travis-ci.org/github/IQSS/dataverse/builds/696173775 as of ff025fd so I'm moving this to QA. Thanks, @johnhuck

Screen Shot 2020-06-08 at 4 20 13 PM

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Looks good.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Community Dev 💻❤️ to QA 🔎✅ Jun 8, 2020
@kcondon
Copy link
Contributor

kcondon commented Jun 9, 2020

@johnhuck This works great, thanks. Do you mind adding a release notes file that indicates that in order for previously published and exported datasets to reflect this correction, they need to be reexported, curl http://localhost:8080/api/admin/metadata/reExportAll

@pdurbin
Copy link
Member

pdurbin commented Jun 9, 2020

@johnhuck if you're not familiar with our release notes process, I would suggest creating a file at the following location:

doc/release-notes/6959-html-codebook.md

You can read more about the process at http://guides.dataverse.org/en/4.20/developers/making-releases.html#write-release-notes

@kcondon kcondon moved this from QA 🔎✅ to Community Dev 💻❤️ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Jun 10, 2020
@johnhuck
Copy link
Contributor Author

@kcondon OK, I will do my best. This is good experience for me!

@pdurbin
Copy link
Member

pdurbin commented Jun 10, 2020

@johnhuck I edited the description...

Screen Shot 2020-06-10 at 3 18 04 PM

... to have "Closes #6959" so that the issue will be closed when the pull request is merged. Here are a couple more screenshots:

Screen Shot 2020-06-10 at 3 18 31 PM

Screen Shot 2020-06-10 at 3 18 55 PM

@johnhuck
Copy link
Contributor Author

johnhuck commented Jun 18, 2020

@pdurbin @kcondon OK, I think this is ready. When you aren't busy with the community meeting, let me know if there's anything else I need to do (I don't know if the failing tests mean anything).

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Community Dev 💻❤️ to QA 🔎✅ Jun 18, 2020
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Looks good.

@kcondon kcondon merged commit 9fa0c2e into IQSS:develop Jun 18, 2020
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Jun 18, 2020
@kcondon kcondon self-assigned this Jun 18, 2020
@johnhuck johnhuck deleted the patch-1 branch June 18, 2020 19:28
@djbrooke djbrooke added this to the Dataverse 5 milestone Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Bug in DDI Codebook Export XSLT
5 participants