Conversation
atarix83
left a comment
There was a problem hiding this comment.
thanks @artlowel @LotteHofstede for this PR
Generally it looks good to me, and it seems to work properly. I only have a couple of comment/suggestion :
- once I visited the processes page list and than started a new process, when i turn back to the processes page the list is not updated, I have to refresh the page. It seems like a cache problem
- the table used to list processes could cover all the page's width (this is a suggestion that could be addressed in a follow-up PR)

- it should be useful to have a refresh button in the processes page, that allows to update the processes' status. (this is a suggestion that could be addressed in a follow-up PR)
|
@artlowel : Just a quick note, this needs a rebase. You also may want to quickly double check this is updated to work against the (recently merged) REST PR DSpace/DSpace#2648. There refactors done there in the last few weeks that may not be applied here, namely the changes in this contract PR about how files are exposed: DSpace/RestContract#99 Since this PR doesn't look to have been updated recently, it may require some updates before it will work again |
…estricted-bitstreams Conflicts: src/app/core/core.module.ts
|
This pull request introduces 9 alerts when merging 1cb0bb7 into bb4aedc - view on LGTM.com new alerts:
|
…estricted-bitstreams Conflicts: src/app/shared/shared.module.ts
|
This pull request fixes 2 alerts when merging 7cfa0f1 into 0e1874d - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
👍 WOW! This is very well written code (thanks for all the code comments/typedocs) and it works/looks great. I tested all three processes (index-discovery, metadata-export and metadata-import) and they all work well. Thanks @LotteHofstede and @artlowel!
That said, I will note I have a few minor usability suggestions (for a separate follow-up PR). These can be copied into an issue ticket (as they need not be done in this PR).
In my opinion, some of the options/flags should be uneditable & preset from the Admin UI (or potentially preset by the REST API, which passes them to the UI layer). Currently, you can add/change any flag, which is powerful, but also could be a slight security issue (all Administrative users would need to have a high level of trust). More specifically...
- The
metadata-exportscript should NOT let you type in a--filelocation. Since it's only able to write a file to the server where DSpace is running, it's unlikely the Admin will know an exact path to type in. Additionally, this could be a security issue if you can write the output file anywhere on the system. Ideally, instead, the file should always be written to the[dspace]/exports/folder (which is where it was written in DSpace 6.x, IIRC). - The
metadata-importscript should NOT let you specify the--emailparam. Instead, it should be autofilled out with your email address & be uneditable. Again, it could be considered a security issue if any Administrator can batch edit metadata as if they were anyone else -- this would bypass all normal authorization checks within DSpace.
After writing both of those out, I now realize these might both be issues at the REST API layer. In any case, I think this PR is good enough as-is, and it's very well done. I'll create these separate issue tickets after we merge this PR & we can schedule them for beta4.
|
@atarix83 : Could you give this a quick look? It's a big PR, but I've found it works well. All its dependencies have been committed & I've just deployed them to the REST demo site...so you can test against that site if it's easiest. |
atarix83
left a comment
There was a problem hiding this comment.
@artlowel @LotteHofstede thanks for the great work and thanks for taking my suggestions into account.
It remain only one suggestion, given in the previous review, that is not addressed :
it should be useful to have a refresh button in the processes page, that allows to update the processes' status. (this is a suggestion that could be addressed in a follow-up PR)
If you think this could be reasonable for you, I'd create a ticket to be addressed in a follow-up PR
|
@atarix83 Sure, that makes sense |
DSC-38 Refactor Approved-by: Vincenzo Mecca
add forgotten var
add forgotten var
This PR adds pages to:
Depends on #716 and DSpace/DSpace#2783
You can start a new process by signing in and selecting New → Process from the sidebar.
You can get an overview of available processes by selecting Processes from the sidebar, from there you can click on an individual process to see its output.
Note that this is an initial version of these UIs. There are a lot of improvements we could make and extra features we could add, but we kept the scope as small as possible in order to keep the effort down.