Curation Task Admin UI

[YanaDePauw]

References

This PR is related to Angular issue 285
REST API PR: DSpace/DSpace#2820

Description

Adds the System wide UI, and the community and collection tab to run curation tasks.

Instructions for Reviewers

System Wide Curation tasks:

• Log in as an admin
• Navigate to the curation page by using the link in the admin side menu
• Select a curation task and enter a handle, or leave it empty to run it system wide.
• Verify that the task is started and that you are redirected to the corresponding process page

Collection/Community Curation tasks:

• Log in as an admin
• Navigate to the edit collection/community page
• Navigate to the curation tab
• Select a curation task
• Verify that the task is started and that you are redirected to the corresponding process page

Note
The process page to which is navigated currently only contains the state of the process (e.g. RUNNING, COMPLETED, ...).
The output of the curation task is not displayed. Displaying the output of a process will be added in a follow-up PR.

Checklist

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
  • Include tests for different user types (if behavior differs), including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
  • Include tests for error scenarios, e.g. when errors/warnings should appear (or buttons should be disabled).
  • For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[tdonohue]

@YanaDePauw : Thanks, the code looks good overall. I've tested this (using the backend: DSpace/DSpace#2820), and it works for what it does. But, I was a bit surprised that there's no way to view the output of the curation task. That looks to be a flaw of the backend though, and I've noted it there: DSpace/DSpace#2820 (review)

It's also odd to me that Curation tasks are runnable both from the Curation Tasks menu item (which is the easier interface) and from the Processes UI (which is harder to use for Curation tasks). I'd expect them to NOT be in the Processes UI, as that seems to bypass the new settings (see below) in environment.common.ts, as all tasks become runnable.

I think we may need more discussion around the goal of Curation Tasks in the UI, as unfortunately they don't seem useful to me at this time. If you can only see the output by looking at the log files, then I don't see a reason to move them off the commandline. So, I'll bring this up for discussion in this week's meeting to see how we can move it forward.

[tdonohue]

These configuration seem to duplicate the ones in curate.cfg: https://github.com/DSpace/DSpace/blob/main/dspace/config/modules/curate.cfg#L31-L33

That's fine if we need to move these configurations to the UI layer, but then we should remove them from curate.cfg on the backend (perhaps in DSpace/DSpace#2820).

[artlowel]

I was a bit surprised that there's no way to view the output of the curation task. That looks to be a flaw of the backend though, and I've noted it there: DSpace/DSpace#2820 (review)
…
I think we may need more discussion around the goal of Curation Tasks in the UI, as unfortunately they don't seem useful to me at this time. If you can only see the output by looking at the log files, then I don't see a reason to move them off the commandline. So, I'll bring this up for discussion in this week's meeting to see how we can move it forward.

There should be a rest PR next week that adds the ability to show the output for all processes, not just curation tasks. The UI already has a section to show it, commented out currently.

It's also odd to me that Curation tasks are runnable both from the Curation Tasks menu item (which is the easier interface) and from the Processes UI (which is harder to use for Curation tasks). I'd expect them to NOT be in the Processes UI, as that seems to bypass the new settings (see below) in environment.common.ts, as all tasks become runnable.

The idea behind the processes UI is that it stands in for the CLI. You're able to run every script that's available on the rest endpoint using a general UI that functions the way a CLI woud work and not something tailored any specific task. Every script that gets a dedicated UI (another example would be #798) will also still be possible from the general processes UI.

There is an argument to be made to limit the curation tasks you are able to execute in the processes UI as well, according to the settings in environment.ts. But then perhaps the question is: why are do we need to be able to have curation tasks that are active but can't be started from the UI?

If it's for security reasons, then you probably shouldn't be able to start them from the rest endpoint either. If the issue is that the dedicated curation task UI doesn't have the fields to specify all the parameters you'd need for certain tasks, then it makes sense that you can use the general process UI to run them, as it has a way to add those parameters.

[tdonohue]

@artlowel : Thanks for the explanation...I was simply not aware that this initial implementation of Curation Tasks was decided to not include the output of the task. So, that followup PR does seem like it'd resolve the issue there.

With regards to which tasks should be runnable, I'm beginning to suspect the issue there is that the backend REST API has too many tasks enabled by default. See my "UPDATE" in this comment on the REST PR. It also seems like the Angular UI never actually checks to see which tasks are available from the backend...they simply must be manually synced between environment.common.ts and the curate.cfg on the backend.

That's likely good enough for now, but we'll have to document it...essentially, the Curation Task UI only lets you run what is in environment.common.ts, but if additional tasks are enabled in curate.cfg, then it's possible (if you know the task names) to run them from the Processes UI only.

UPDATE: I'll rereview this, but I'd appreciate it if we change the description to note that it does NOT provide output of the curation tasks, and that will come later in a followup PR to the Processes UI.

[tdonohue]

Closing & reopening this PR to try and re-trigger Travis CI. For some reason it never ran on the recent (force pushed) updates to this PR.

[artlowel]

UPDATE: I'll rereview this, but I'd appreciate it if we change the description to note that it does NOT provide output of the curation tasks, and that will come later in a followup PR to the Processes UI

Done

[tdonohue]

@YanaDePauw and @artlowel : Just a quick note. After looking again at both the corresponding REST PR (DSpace/DSpace#2820) and this one, I think we should (as Art noted in today's meeting) replace/remove the changes to environment.common.ts in this PR.

Instead, ideally, this PR should get the list of enabled tasks from the /api/config/properties/plugin.named.org.dspace.curate.CurationTask property exposed by the REST API (in PR 2820). You still would need to configure more user friendly names for these tasks (or perhaps talk to Jonas & Kevin about passing more user friendly names from the REST API again via /api/config/properties, if you think that'd be easier). That would avoid having to duplicate a list of "enabled" curation tasks in both the UI and REST API. That would also address one of my major concerns, as the same tasks would then appear in both this Curation Task UI and in the Processes UI.

I'll give this another test/review tomorrow, but wanted to add in this note about a way to move this forward.

[tdonohue]

After re-reviewing this, I think the code looks good based on the scope of this PR. However, as I noted above, I think ideally we should be getting the list of enabled Curation Tasks from the REST API. So, if possible, in this PR, I'd like to consider removing the changes to environment.common.ts, and instead retrieving that list of enabled tasks from the REST API (the *.label i18n keys can still remain in the Angular UI however, depending on whether you feel they are easier to manage/translate in the UI or if you'd rather the REST API pass them along somehow)

[crosenbeck]

@YanaDePauw Thank you for work on this PR and overall it looks good. It seems that the first sentence in the note gives a better description of the PR than what is in the description. Could the description be reworded? The note becomes more confusing than helpful to me. I agree with @tdonohue about retrieving the lists of enabled tasks from REST API and remove changes to environment.common.ts file.

[lgtm-com]

This pull request introduces 4 alerts when merging 76ad72f into 8b639bc - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 4 alerts when merging 90f68ee into 8b639bc - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[tdonohue]

👍 Thanks @YanaDePauw for the updates to this PR, and for getting it to request the curation tasks from the REST API. I've tested this alongside the backend REST PR (DSpace/DSpace#2820) and it's working as expected/described. As previously discussed, the task output is unavailable, but that's being worked on in a separate PR.

So, this looks good to me now!

[crosenbeck]

@YanaDePauw Thank you for your work and changes on this PR. The changes look good to me.