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

Support for process output on detail page #827

Merged
merged 7 commits into from Nov 20, 2020

Conversation

MarieVerdonck
Copy link
Contributor

@MarieVerdonck MarieVerdonck commented Aug 4, 2020

References

Description

This PR shows the output of a Process on the process' detail page at {dspaceUI}/processes/

Instructions for Reviewers

The logs of the process are are a list of strings, shown underneath each other is a < pre > section.
If the process has no ProcessOutput or the logs in the ProcessOutput is just an empty array, then a message is shown.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • 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.

@lgtm-com
Copy link

lgtm-com bot commented Aug 4, 2020

This pull request introduces 1 alert when merging 8325a34 into eb98098 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@MarieVerdonck MarieVerdonck marked this pull request as ready for review August 4, 2020 13:02
@artlowel artlowel added this to Needs Reviewers Assigned in DSpace 7 Beta 4 Aug 4, 2020
@artlowel artlowel added the 1 APPROVAL pull request only requires a single approval to merge label Aug 4, 2020
@heathergreerklein heathergreerklein moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 4 Aug 4, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@MarieVerdonck : Thanks for this PR! I gave this a code review today, and the code looks good (just one minor inline comment below).

Overall, it looks like it should work, however I was unable to test it as the backend PR (DSpace/DSpace#2905) is not working to build/install today (it throws errors when I try to run it). So, without a backend to test against, I was only able to do a code review.

As I'm about to leave on holiday for a week, I'll note that I'm +1 for this PR as-is provided that someone else can test it & verify it works against the backend once that backend PR is fixed/updated (as noted in this comment, that PR is planned to be refactored anyhow to change the implementation).

src/app/core/data/process-output-data.service.ts Outdated Show resolved Hide resolved
@tdonohue tdonohue self-requested a review August 27, 2020 14:53
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@MarieVerdonck : Code here looks good overall. But, when I test it alongside the REST PR (DSpace/DSpace#2934), every request for a single Process (UI Path: /processes/[id])results in a "404 OK" error from the backend.

It looks to me like the request being sent from the UI might be invalid, as the embed=output param doesn't seem to work from the REST API either. So, here's what the request looks like that results in a 404: http://localhost:8080/server/api/system/processes/4?embed=script&embed=output

I'm also not sure whether we should always embed the entire output...as some of the output might be large in size. So, it seems like we should have a way to view the simple information about the process prior to opening the output. But, maybe I'm misunderstanding the purpose of that command.

@tdonohue tdonohue self-requested a review September 3, 2020 14:48
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@MarieVerdonck : I re-tested this with the backend PR (DSpace/DSpace#2934). It's not quite working as expected, though it's no longer throwing obvious errors.

Here's the behavior I see:

  • First, I kick off a new process from http://localhost:4000/processes/
  • I immediately click on the new process while the process is in the "Running" status.
    • Bug: While the process is "running", I see the Process Output section & "Retrieve process output" button...even though no output is yet available.
  • Bug: Once the process is "Completed", I click on the "Retrieve process output" and get a response that "no process output is available". But, if I look at the Chrome DevTools, I see that the 200 response included a "content" link which allows me to download the content directly from the REST API. So, it looks like the UI is not finding that "content" link.
  • Bug: If I go to an old process (one with no output), and click on "Retrieve process output", I just see the "Retrieving..." message forever. If I look at Chrome DevTools, I see it responded with a 204 No Content response, but it looks like the UI didn't handle that response.

@tdonohue tdonohue removed this from Under Review in DSpace 7 Beta 4 Sep 24, 2020
@tdonohue tdonohue added this to Needs Reviewers Assigned in DSpace 7 Beta 5 via automation Sep 24, 2020
@tdonohue tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 5 Sep 24, 2020
@tdonohue tdonohue added this to the 7.0beta5 milestone Sep 24, 2020
@tdonohue tdonohue moved this from Under Review to Stalled/On Hold in DSpace 7 Beta 5 Oct 15, 2020
@tdonohue
Copy link
Member

tdonohue commented Nov 4, 2020

@MarieVerdonck : Could you rebase this PR on the latest code? It looks like there's a small merge conflict here now & I'd like to be able to re-test this PR with the new backend PR (DSpace/DSpace#3009)

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 @MarieVerdonck : I've re-reviewed & retested this today with the new backend PR (DSpace/DSpace#3009) and it works exactly as described (all my previously reported issues are gone as well). The code looks good too. Thanks!

DSpace 7 Beta 5 automation moved this from Stalled/On Hold to Reviewer Approved Nov 20, 2020
DSpace 7 Beta 5 automation moved this from Reviewer Approved to Done Nov 20, 2020
@tdonohue tdonohue reopened this Nov 20, 2020
DSpace 7 Beta 5 automation moved this from Done to Needs Reviewers Assigned Nov 20, 2020
@tdonohue tdonohue merged commit f1b1f81 into DSpace:main Nov 20, 2020
DSpace 7 Beta 5 automation moved this from Needs Reviewers Assigned to Done Nov 20, 2020
DSpace 7 Beta 5 automation moved this from Done to Under Review Apr 15, 2021
DSpace 7 Beta 5 automation moved this from Under Review to Reviewer Approved Apr 15, 2021
DSpace 7 Beta 5 automation moved this from Reviewer Approved to Done Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants