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

Minor bug fixes, including fix for null pointers in the new license selector that did not work with licenses without image URI. #8499

Closed
wants to merge 7 commits into from

Conversation

ErykKul
Copy link
Collaborator

@ErykKul ErykKul commented Mar 17, 2022

What this PR does / why we need it:
I am new for this project and I have run against some problems when trying out the development branch locally. More in particular some validation errors in NetBeans, as well as some errors when running the application and/or tests. I have fixed them and thought to share it with you.

Which issue(s) this PR closes:
I did not see any specific issues reported for the problems I did run against:

  • License selector did not work for licenses without image URI (null pointer exceptions)
  • DDIExporterTest.java crashed on unmarshalling of LocalDateTime class
  • NetBeans complained on namespace declarations in some of the xhtml files
  • File upload javascript was giving errors for not unique identifier

Closes #

Special notes for your reviewer:

  • I am very new to this project, started one week ago, I might have missed something, etc.

Suggestions on how to test this:

  • Uploading licenses without image URL and trying to select them in license selector.

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.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 17, 2022

There is still a problem after saving the dataset with a license that does not contain an image URL, page cannot be rendered.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 17, 2022

I have just fixed the remaining null pointers, the dataset page opens now also when license without image url was selected.

@qqmyers
Copy link
Member

qqmyers commented Mar 17, 2022

@ErykKul - thanks for the PR, and welcome! FWIW: We're trying to get v5.10 out the door and there was a quick decision to grab the iconURL-related fixes you've made, so I went ahead and cherry picked your two commits for that into #8504. Usually we'd just comment back and ask you to split things up. In any case, I think I/others will be looking a bit more at your other fixes here so those will probably hit the next release.

@pdurbin
Copy link
Member

pdurbin commented Mar 18, 2022

@ErykKul hi! PR #8504 has been merged, which should address your concerns about icons in Licenses. Definitely a bug. Thanks for the heads up and the commits!

For the remaining items, do you mind creating issues (and pull requests, if you feel like it) for each one? These:

  • DDIExporterTest.java crashed on unmarshalling of LocalDateTime class
  • NetBeans complained on namespace declarations in some of the xhtml files
  • File upload javascript was giving errors for not unique identifier

Whenever possible, we try to work in small chunks. We find that work moves more quickly across our project board when small pull requests address small issues and can be reviewed, QA'ed, and merged independently.

@scolapasta scolapasta moved this from Review 🦁 to Community Dev 💻❤️ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Mar 18, 2022
@coveralls
Copy link

Coverage Status

Coverage remained the same at 18.864% when pulling 4e9dacc on ErykKul:develop into 27a733f on IQSS:develop.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 21, 2022

@pdurbin

I have created new pull requests. I have dropped this one:

  • NetBeans complained on namespace declarations in some of the xhtml files

It did not fix anything and broke style import.

The other two pull requests:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants