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

As a superuser, I want to make metadata updates without changing dataset version #4760

Closed
adam3smith opened this issue Jun 19, 2018 · 39 comments

Comments

@adam3smith
Copy link
Contributor

Originally posted to Community list where @pdurbin linked to a previous request on the list for similar functionality.

There are two main use cases for us:

  1. We want to add publication metadata (e.g. studies referencing and/or using the dataset) after the dataset is published. This is fairly common
  2. We may want to fix or further enhance metadata after initial publication

Especially 1.) can occur many times over the lifespan of a dataset and we think bumping up the version number, even with "minor version bump", especially when you see something like "Version 1.12" for replication data, may cause confusion/wasted time at best (with people spending time checking changes between versions) and suspicion at worst ("Why was this replication data update after the fact?").

I understand there are concerns especially among people running DV as a self-service repository, so this needs to be limited to the right permissions. On our end, we would be happy with limiting this to super users, but I can see the case for groups wanting to do this within their own (sub-)Dataverses, so there may be a case for allowing this for Dataverse admins but I have no strong feelings about this either way.

@pdurbin pdurbin changed the title As a curator/admin/superuser, I want to make metadata updates without chaning dataset version As a curator/admin/superuser, I want to make metadata updates without changing dataset version Jul 3, 2018
@djbrooke
Copy link
Contributor

djbrooke commented Aug 1, 2018

Thanks @adam3smith. I ran this by @mercecrosas and she's OK with it.

As far as implementation, I'm thinking API only and no front end. Do you (or anyone else out there) have any thoughts on this approach?

@adam3smith
Copy link
Contributor Author

Thanks for checking. API-only works for us

@djbrooke
Copy link
Contributor

djbrooke commented Aug 6, 2018

@scolapasta - I have this one as ready to be estimated and I'd like to get your thoughts on the architecture here. Would this be a separate API endpoint or would the "no version update" be something that's passed as part of a request to a current endpoint? Or something completely different? I feel like the latter would be more scalable, but I'm not familiar enough with this. Thanks!

@adam3smith
Copy link
Contributor Author

@djbrooke trying to follow the status changes on this ticket -- could you briefly say what the status is here?

@pdurbin
Copy link
Member

pdurbin commented Oct 9, 2018

@adam3smith hi, @djbrooke is on vacation but by looking at https://waffle.io/IQSS/dataverse I can see that it's under the "Ready" column (recently renamed from "Backlog" so we should also update the GitHub label, changing it from "Backlog" to "Ready") but it hasn't yet been estimated. That is to say, it hasn't yet been given a "size" in Waffle. We give issues a size/estimate during sprint planning and backlog grooming meetings on Wednesday afternoons. I'm not sure when we'll estimate this one. If you think it would be helpful for us to write up a little more on our process in the dev guide or CONTRIBUTING.md or elsewhere, please let us know! A separate issue would be best. Thanks!

@adam3smith
Copy link
Contributor Author

Thanks -- that's actually how I (think I) understood the process, but then I would have expected the "ready for estimation" label to stay on the issue until it was estimated? Or did you just retire that label?

@pdurbin
Copy link
Member

pdurbin commented Oct 9, 2018

The "ready for estimation" label is new and still in use. The pattern so far has been to add "ready for estimation" and then assign it to someone who who explain the issue during sprint planning or backlog grooming so that we can estimate the effort (size in Waffle).

@adam3smith
Copy link
Contributor Author

@pdurbin & @djbrooke -- has IQSS done any work on this? We'd otherwise likely want to put this on our Q1 roadmap. I think @qqmyers had some initial thoughts on a simple GUI integration unless you're dead set against that? I think the concerns with this for self-deposit repositories could be addressed by making it superuser only

@djbrooke
Copy link
Contributor

djbrooke commented Jan 2, 2019

Thanks @adam3smith for the ping. @scolapasta - let's check in about this later this week

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - It looks like you have the Archiver partially set up:

I think you've got :ArchiverClassName set to the DuraCloud class, but don't have the java properties for duracloud.username and maybe duracloud.password set in glassfish. Since we've dropped the DuraCloud credentials, I think if you delete the :ArchiverClassName setting, you should be able to publish and/or update the current version.

If you want to test with archiving, I think we have to recreate a duracloud account. (The insides of the Archiver classes haven't changed, so nominally they should not be affected by this PR. And I know the archiving works here with update current version...)

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

Also looks like I missed getting the success/fail messages in Bundle - I'll add those now...

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers OK, I can save current version and page updates without bundle error. I've removed Archiver and DuraCloud settings but am still seeing a server log error when I update current version:

[2019-02-28T16:42:48.299-0500] [glassfish 4.1] [WARNING] [] [edu.harvard.iq.dataverse.util.ArchiverUtil] [tid: _ThreadID=51 _ThreadName=jk-connector(2)] [timeMillis: 1551390168299] [levelValue: 900] [[
Unable to instantiate an Archiver of class: null]]

[2019-02-28T16:42:48.300-0500] [glassfish 4.1] [SEVERE] [] [] [tid: _ThreadID=51 _ThreadName=Thread-9] [timeMillis: 1551390168300] [levelValue: 1000] [[
java.lang.NullPointerException
at java.lang.Class.forName0(Native Method)
at java.lang.Class.forName(Class.java:264)
at edu.harvard.iq.dataverse.util.ArchiverUtil.createSubmitToArchiveCommand(ArchiverUtil.java:25)
at edu.harvard.iq.dataverse.DatasetPage.updateCurrentVersion(DatasetPage.java:2014)
at edu.harvard.iq.dataverse.DatasetPage.releaseDraft(DatasetPage.java:1797)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.sun.el.parser.AstValue.invoke(AstValue.java:289)
at com.sun.el.MethodExpressionImpl.invoke(MethodExpressionImpl.java:304)
at org.jboss.weld.util.el.ForwardingMethodExpression.invoke(ForwardingMethodExpression.java:40)
at org.jboss.weld.el.WeldMethodExpression.invoke(WeldMethodExpression.java:50)
at com.sun.faces.facelets.el.TagMethodExpression.invoke(TagMethodExpression.java:105)
at javax.faces.component.MethodBindingMethodExpressionAdapter.invoke(MethodBindingMethodExpressionAdapter.java:87)
at com.sun.faces.application.ActionListenerImpl.processAction(ActionListenerImpl.java:102)
at javax.faces.component.UICommand.broadcast(UICommand.java:315)
at javax.faces.component.UIViewRoot.broadcastEvents(UIViewRoot.java:790)
at javax.faces.component.UIViewRoot.processApplication(UIViewRoot.java:1282)
at com.sun.faces.lifecycle.InvokeApplicationPhase.execute(InvokeApplicationPhase.java:81)
at com.sun.faces.lifecycle.Phase.doPhase(Phase.java:101)
at com.sun.faces.lifecycle.LifecycleImpl.execute(LifecycleImpl.java:198)
at javax.faces.webapp.FacesServlet.service(FacesServlet.java:646)
at org.apache.catalina.core.StandardWrapper.service(StandardWrapper.java:1682)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:344)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:214)
at org.glassfish.tyrus.servlet.TyrusServletFilter.doFilter(TyrusServletFilter.java:295)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:256)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:214)
at org.ocpsoft.rewrite.servlet.RewriteFilter.doFilter(RewriteFilter.java:226)
at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:256)
at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:214)
at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:316)
at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:160)
at org.apache.catalina.core.StandardPipeline.doInvoke(StandardPipeline.java:734)
at org.apache.catalina.core.StandardPipeline.invoke(StandardPipeline.java:673)
at com.sun.enterprise.web.WebPipeline.invoke(WebPipeline.java:99)
at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:174)
at org.apache.catalina.core.StandardPipeline.doInvoke(StandardPipeline.java:734)
at org.apache.catalina.core.StandardPipeline.invoke(StandardPipeline.java:673)
at org.apache.catalina.connector.CoyoteAdapter.doService(CoyoteAdapter.java:412)
at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:282)
at com.sun.enterprise.v3.services.impl.ContainerMapper$HttpHandlerCallable.call(ContainerMapper.java:459)
at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:167)
at org.glassfish.grizzly.http.server.HttpHandler.runService(HttpHandler.java:201)
at org.glassfish.grizzly.http.server.HttpHandler.doHandle(HttpHandler.java:175)
at org.glassfish.grizzly.http.server.HttpServerFilter.handleRead(HttpServerFilter.java:235)
at org.glassfish.grizzly.filterchain.ExecutorResolver$9.execute(ExecutorResolver.java:119)
at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeFilter(DefaultFilterChain.java:284)
at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeChainPart(DefaultFilterChain.java:201)
at org.glassfish.grizzly.filterchain.DefaultFilterChain.execute(DefaultFilterChain.java:133)
at org.glassfish.grizzly.filterchain.DefaultFilterChain.process(DefaultFilterChain.java:112)
at org.glassfish.grizzly.ProcessorExecutor.execute(ProcessorExecutor.java:77)
at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:561)
at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:112)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:117)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.access$100(WorkerThreadIOStrategy.java:56)
at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy$WorkerThreadRunnable.run(WorkerThreadIOStrategy.java:137)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:565)
at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:545)
at java.lang.Thread.run(Thread.java:748)]]

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - since there are two possible transactions (the update of the version and, separately, an archive submission, if configured), there are four possible outcomes - success/fail of step 1 and success of step 1 with success/fail of the second. The first should succeed unless you make parallel edits in another browser, so I think you could test the fail path if desired. The way you were configured before - with :ArchiverClassName set is a way to test the update version succeeds/archiving step fails path. The only one that requires more to test would be setting up Duracloud to make the archiving also succeed (but again, that's the one we're set up for.)

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - I think that's #5590 - archiving fails when it isn't set up. I think that also shows up in the log for any publish without archiving turned on - non-fatal, but log pollution...

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

Would you fix #5590 as part of this pr? I'm not seeing on 4.11 branch

I don't have workflows defined according to:
curl http://localhost:8080/api/admin/workflows

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - fix added to #5541. You're right that (thankfully) v4.11 only throws this in the log if you have set up the workflow or call the archive api, so you're probably the only one I've caused trouble for...

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers Updating a file to be restricted or unrestricted allows updating in current version but fails with error in UI, not in server log.
Error – Dataset Version Update failed. Changes are still in the DRAFT version. - edu.harvard.iq.dataverse.engine.command.exception.IllegalCommandException: Cannot change files in the dataset If you believe this is an error, please contact Harvard Dataverse Support for assistance.

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers Did you make any change to the publish dataset api for this? How would one specify current version?

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - if I try this in v4.11-qdr, I only see the 'major' version option. I don't think I changed anything w.r.t. the decision to allow a minor version (DatasetVersion.isMinorUpdate()), so you may have found a bug there. Can you get to the minor version option doing a restrict/unrestrict on dev or v4.11?

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - yes - you can provide a type updatecurrent instead of major or minor in the publishDataset api call.

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers Yes, I can see minor option in 4.11 when changing file restriction. Is the doc updated for the api change?

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - my bad w.r.t the major version - my test dataset had an extra file. When I just restrict/unrestrict, I see the update current option. It works for me, so I'll look at what may be different than in the PR.
Will also update the doc for the api...

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers Thanks, for what it's worth, updatecurrent for file restriction works for the publish api.

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - hmm - the only way to get the IllegalCommandException is to get false in the if statement on line 96 of CuratePublishedDatasetVersionCommand.java:
if (draftFmd != null && publishedFmd != null) {

That means that somehow the filemetadata for the draft or prior published version was not found - versus anything being wrong with the logic to set the restricted state. Are you able to repeat the problem? Any thoughts on how/why the filemetadata might not be there? (perhaps some sequence of events with no page refreshes leaves a stale state I'm not catching?) I'll look for a way to trigger the problem but any clue you can find w.r.t. it being occasional/only under certain circumstances would be helpful...

pdurbin added a commit to QualitativeDataRepository/dataverse that referenced this issue Feb 28, 2019
@pdurbin
Copy link
Member

pdurbin commented Feb 28, 2019

@qqmyers and standup I asked if there are any docs. Nope. I stubbed out some at 174d9ce

It looks like this new feature is available via API as well.

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@qqmyers I am now unable to reproduce the issue. So, I'm ok with this as is. I'll try it a couple more times to be sure but then, after docs are added, I will merge it.

pdurbin added a commit to QualitativeDataRepository/dataverse that referenced this issue Feb 28, 2019
@pdurbin
Copy link
Member

pdurbin commented Feb 28, 2019

@kcondon did you test via API? I just documented it at cd66f3f

@kcondon
Copy link
Contributor

kcondon commented Feb 28, 2019

@pdurbin Haven't you been reading the comments? ;)

@pdurbin
Copy link
Member

pdurbin commented Feb 28, 2019

@kcondon ah, now I see some chatter about "updatecurrent" API testing. Good! 😄

@qqmyers
Copy link
Member

qqmyers commented Feb 28, 2019

@kcondon - I made a minor add to @pdurbin docs (thanks for that!), so I guess its good to go. I am concerned that you had that one error. My only guess right now is that there's some way to get a stale page. If that's true, I should be able to refresh from the db to fix it, but I don't want to to do that if we can't duplicate the problem. On the good side - if it is something stale, a page refresh will fix it, so there's a work around...

-- Thanks!

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

No branches or pull requests

7 participants