-
Notifications
You must be signed in to change notification settings - Fork 433
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
Feature process polling #2480
Feature process polling #2480
Conversation
…DetailComponent, and the ProcessDataService tests
… process instead of a second request
…he followLinks Also fixed the mock values used by findListByHref, because they didn't use PaginatedLists but regular items
# Conflicts: # src/app/core/data/processes/process-data.service.ts
… when the lastUpdated didn't change
Hi @KoenP, |
…ling # Conflicts: # src/app/core/data/base/base-data.service.spec.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KoenP : Gave this a review & test (alongside the required backend PR). Overall, the code looks good and the test was successful. However, I've found the "Delete process" button now results in a 404 error. This is not reproducible on https://sandbox.dspace.org/, so I believe it is caused by this PR (or maybe the backend PR?)
Here's how to reproduce the issue:
- Login as an Admin, and run a Process (or find one you've run in the past)
- Click on the Process to get to it's detail page (
[dspace.ui.url]/processes/[id]
). - On the detail page, click "Delete Processes"
- The deletion will succeed, but you'll see a 404 error page. It appears to be caused because you remain on the process detail page instead of being sent back to the Processes Overview (
[dspace.ui.url]/processes
)
Beyond that bug, everything else looks great. I can also verify that the polling is much smoother. The page doesn't have to continually reload/redraw until the process is completed.
@tdonohue: Thnx for the review! I fixed the error, but I also found out that visiting a non-existing process pages (e.x. https://sandbox.dspace.org/processes/10000) throws a |
@KoenP or @alexandrevryghem : Please rebase this on |
@KoenP and @alexandrevryghem : I saw that the e2e tests failed again, so I thought I'd try them locally. I can reproduce the failure locally in this scenario:
I can verify this same behavior does NOT occur on |
@KoenP and @alexandrevryghem : As noted in today's Dev Meeting to @artlowel , the issue I identified above may be in this PR (possibly in some of the changes to If I run this frontend PR with the So, I believe the issue might be in some of the changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @KoenP and @alexandrevryghem ! Verified this is working correctly now. Tested the Process Detail is refreshing properly & that Collection Source import process is working well. Code looks good too.
(Also verified locally that yarn e2e
is no longer timing out..so the e2e test issues appear fixed as well.)
This can be merged as soon as the backend PR DSpace/DSpace#9241 is ready. (it needs some updates based on feedback & based on recent changes to main
)
Backend PR is complete. Re-tested this today, and it still works well. Merging immediately. |
References
s
DSpace#9241Description
Adds an
autoRefreshUntilCompletion
method toProcessDataService
that is intended to be used anywhere where we request the current status of a process repeatedly until it is done. Also replaces ad-hoc implementations of this concept around the code base with this method. This method also allows you to passfollowLinks
to it for performance reasons.During the making of this PR an additional bug was discovered, the cached object retrieved using a
followLink
s currently aren't automatically marked as stale when you invalidate the cached object with whom you requested them.Instructions for Reviewers
List of changes in this PR:
ProcessDataService
obtains a new methodautoRefreshUntilCompletion
, which transforms a process ID into anObservable<RemoteData<Process>>
for that process ID. A second optional parameter allows for the polling interval to be configured.CollectionSourceControlsComponent
was adjusted to use this method instead of its own implementation.ProcessDetailComponent
was adjusted to use this method instead of its own implementation.This feature is probably most easily tested by verifying that
CollectionSourceControlsComponent
andProcessDetailComponent
still work as before.Checklist
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.