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

IQSS-4778 generate DOIs only for published files #4779

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jun 23, 2018

New Contributors

Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!

Related Issues

Pull Request Checklist

@coveralls
Copy link

coveralls commented Jun 23, 2018

Coverage Status

Coverage decreased (-0.001%) to 13.236% when pulling 2f7b0ae on QualitativeDataRepository:4778-registerDataFileAll_for_published_files_only into 51e67e6 on IQSS:develop.

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.

Please see comments inline.

if (df.getIdentifier() == null || df.getIdentifier().isEmpty()) {
User u =findUserOrDie();
if ((df.getIdentifier() == null || df.getIdentifier().isEmpty())&& df.isReleased()) {
User u =findAuthenticatedUserOrDie();
Copy link
Member

Choose a reason for hiding this comment

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

I just spoke with @sekmiller about adding the check for isReleased and we agree that it's more consistent how new datasets are created and published. I'm sort of wondering why there was a change to findUserOrDie. Also, should these changes be made to the registerDataFile method above as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pdurbin - Sorry - the findAuthenticatedUser is a separate issue (which I meant to remove from this pull). Since I didn't - let me explain:

findUserOrDie appears to first check the submitted key to see if it from a privateUrl and then call findAuthenticatedUserOrDie (and if there's no key, it returns the guest user). Since a privateUrl wouldn't work for multiple datasets and a guest user would also not be allowed, it seemed that calling findAuthenticatedUserOrDie() directly would be better. I have not checked to see what happens with the original code if no key is sent or a privateURL is used - I assume permissions gets checked somewhere along the way - but it seemed simpler to just reject early if a valid superuser apikey was not available.

I think the same change could/should be made to registerDataFile, unless using a privateUrl was intended to be OK there.

Feel free to edit directly or let me know and I can remove/add it to registerDataFile...

Copy link
Member

Choose a reason for hiding this comment

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

@qqmyers thanks, I haven't been very close to this part of the code so I'm fine with others weighing in on whether the pull request needs more work or not.

@kcondon
Copy link
Contributor

kcondon commented Jun 29, 2018

@qqmyers Do you mind refreshing from /develop? We've recently updated the version of the war file. Thanks!

@pdurbin
Copy link
Member

pdurbin commented Jul 2, 2018

@kcondon done (by @qqmyers ) in abfe0fd

@kcondon
Copy link
Contributor

kcondon commented Jul 2, 2018

@qqmyers thanks for updating. I've tested it and it works as described but do you mind making a tweak to the logging on start and end counts?

For example, I have 7 files that are unregistered: 3 published, 3 unpublished, 1 harvested. You correctly register the published ones and leave the others alone. However, the start count and end count messages are a bit confusing:

[2018-07-02T13:56:18.775-0400] [glassfish 4.1] [INFO] [] [edu.harvard.iq.dataverse.api.Admin] [tid:
_ThreadID=34 _ThreadName=http-listener-1(5)] [timeMillis: 1530554178775] [levelValue: 800] [[
Starting to register 7 files. Mon Jul 02 13:56:18 EDT 2018]]
[2018-07-02T13:56:23.912-0400] [glassfish 4.1] [INFO] [] [edu.harvard.iq.dataverse.api.Admin] [tid: _ThreadID=34 _ThreadName=http-listener-1(5)] [timeMillis: 1530554183912] [levelValue: 800] [[
4 of 7 files were already registered. Mon Jul 02 13:56:23 EDT 2018]]
[2018-07-02T13:56:23.912-0400] [glassfish 4.1] [INFO] [] [edu.harvard.iq.dataverse.api.Admin] [tid: _ThreadID=34 _ThreadName=http-listener-1(5)] [timeMillis: 1530554183912] [levelValue: 800] [[
3 of 7 files registered successfully. Mon Jul 02 13:56:23 EDT 2018]]

It's up to you but perhaps it could say:
7 files are unregistered. Only unregistered, published files will be registered.

and then ending it could day:
4 of 7 files registration was not attempted.
3 of 7 files registered successfully.

@qqmyers
Copy link
Member Author

qqmyers commented Jul 2, 2018

Sure - I'll take a look.

report # of successes versus how many unregistered but published files
there are, along with the total file count.
@qqmyers
Copy link
Member Author

qqmyers commented Jul 2, 2018

@kcondon - see what you think. Since the # of unregistered, published files is unknown to start (unless we loop through all of them to count), the incremental counts are out of the overall total, but I'm logging all three (already registered, not yet published, and to register) as they are processed and then report all three categories at the end, along with the # of successes out of the # that should have been registered.

@kcondon
Copy link
Contributor

kcondon commented Jul 2, 2018

OK will check it out. Thanks!

@kcondon
Copy link
Contributor

kcondon commented Jul 2, 2018

That looks great, thanks!

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.

5 participants