Workflow enhancements

[qqmyers]

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

• Workflow Enhancements #5044- Workflow Enhancements

Pull Request Checklist

• Unit tests completed
• Integration tests: None
• Deployment requirements, SQL updates, Solr updates, etc.: None
• Documentation completed
• Merged latest from "develop" branch and resolved conflicts

[coveralls]

Coverage decreased (-0.01%) to 15.619% when pulling 6d23d6a on QualitativeDataRepository:IQSS/5044-Workflow_Enhnacements into 296e22a on IQSS:develop.

[michbarsinai]

All in all looks good. I didn't get two things:

• Why the direct lock usage? I know JPA is a pain this setting (it was when writing the original version, at least), but the solution of directly playing around with locks will fire back once the lock mechanism change. Is it possible to use the system? If the problems are JPA-related, maybe even instantiate the involved service beans and pass the entity manager directly?
• Why the removal of the DOI provider?

[qqmyers]

The direct lock usage is the way it was. The only thing I changed was to the way the lock is removed since the dataset is no longer find()-ed from the database during the refresh() call and hence the prior remove operation was failing. Overall, the EE/JPA aspects of this are not something I'm overly familiar with and I didn't want to change more than I needed to that might also affect the original workflow design (each step in its own transaction, etc.). However, I don't think I tried reverting to use the addlock command (that was already commented out in the code and perhaps wasn't working due to the way the dataset was being passed and find()-ed before), so maybe that's worth a test - to do both lock and unlock through the commands.

As for removing the doiprovider - to start, I couldn't find anywhere that it was actually being used. Beyond that, once I added the means to get any setting, I figured that would be a better mechanism for any workflow that did require the doiprovider to get its value.

[scolapasta]

@qqmyers I'm going to assign back to you and move back to Community Dev. If you could test the using of commands for the locks, that would be great. If you are don't have the time, no worries, we can live with it as is (since it was already that way), but I think it makes sense to try now if you can.

[michbarsinai]

Sorry for that late reply. I’m agree with Gustavo - that would be a big plus. Other than that, the PR is good to go.

…

Sent from my iPhone

On 18 Sep 2018, at 18:09, Gustavo Durand ***@***.***> wrote: @qqmyers I'm going to assign back to you and move back to Community Dev. If you could test the using of commands for the locks, that would be great. If you are don't have the time, no worries, we can live with it as is (since it was already that way), but I think it makes sense to try now if you can. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[qqmyers]

@michbarsinai, @scolapasta - I've tried flipping back to using the add and remove lock commands and I get org.eclipse.persistence.exceptions.OptimisticLockExceptions when I do. My potentially incorrect understanding here is that, since we're in the middle of a workflow transaction, calling the lock commands, which require new transactions, cause an issue when they try to update the dataset object (I've tried with the in-memory object as well as doing a find by id to get a new dataset object from the db). By only writing a lock to the database and not updating the dataset object involved, and/or not creating new transactions, the local lock/unlock commands get around this while still correctly setting/removing the lock. (I have not run an external workflow, but did test commenting out the unlock command and verifying that when my local workflow finishes, the lock is in the database, which is what should happen when an external workflow happens. (For a local workflow, the lock is removed when the workflow completes - external ones only get to that point once the callback has occurred.)
So - I don't have any good ideas about how the lock commands might be adapted at this point. Unless the above description gives one of you some ideas, I think the current PR should work for all the workflow types.

[pdurbin]

Closing in favor of #5224 which doesn't change the permissions (chmod) of some files.