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

Feature process polling #2480

Merged
merged 25 commits into from
Feb 22, 2024
Merged

Feature process polling #2480

merged 25 commits into from
Feb 22, 2024

Conversation

KoenP
Copy link

@KoenP KoenP commented Sep 7, 2023

References

Description

Adds an autoRefreshUntilCompletion method to ProcessDataService 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 pass followLinks to it for performance reasons.
During the making of this PR an additional bug was discovered, the cached object retrieved using a followLinks 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 method autoRefreshUntilCompletion, which transforms a process ID into an Observable<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 and ProcessDetailComponent still work as before.

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 ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • 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 or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@KoenP KoenP marked this pull request as ready for review September 8, 2023 12:21
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Dec 14, 2023
…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
@artlowel artlowel assigned alexandrevryghem and unassigned KoenP Jan 5, 2024
Copy link

github-actions bot commented Jan 8, 2024

Hi @KoenP,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

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.

@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:

  1. Login as an Admin, and run a Process (or find one you've run in the past)
  2. Click on the Process to get to it's detail page ([dspace.ui.url]/processes/[id]).
  3. On the detail page, click "Delete Processes"
  4. 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.

@alexandrevryghem
Copy link
Member

@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 504 Gateway Time-out error. I also fixed that issue, and I committed it in a separate commit. This way it should be fairly easy to cherry-pick it on dspace-7.x if you want that fix there too.

@tdonohue
Copy link
Member

tdonohue commented Feb 13, 2024

@KoenP or @alexandrevryghem : Please rebase this on main. It's possible that the e2e test failures impacting this PR were just fixed by the merger of #2722 into main.

@tdonohue
Copy link
Member

@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:

  1. Install the backend PR this depends on: Fixed embedding data not working on model objects ending with an s DSpace#9241
  2. Install this PR
  3. Run yarn e2e. This will eventually spin up Cypress to run the tests. Try out these two which are failing frequently in GitHub:
    • browse-by-dateissued.cy.ts - This fails locally as well. It appears to be an issue with the Browse by Date Issued page loading very slowly. It does eventually load, but it takes so long that the e2e test times out. (Because it's a race condition, I'm sometimes able to get this to succeed. But usually, it fails for me locally)
    • browse-by-title.cy.ts - Same scenario. This appears to be a test timeout issue. The Browse by Title page loads very slowly. (Same here, sometimes I'll get lucky and this will succeed. But most of the time it fails for me.)
    • This same issue does NOT occur for browse-by-author or browse-by-subject. Those pages load more quickly. It seems to be specific to browse pages which immediately display Item objects.

I can verify this same behavior does NOT occur on main. On main, both of these pages load very rapidly in yarn e2e and I never see these timeouts. So, it appears to be specific to these PRs. I'm not sure myself what the cause may be, and whether it's this frontend PR or the backend PR.

@tdonohue
Copy link
Member

@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 base-data.service)?

If I run this frontend PR with the main backend, I can still reproduce the odd slowness in some e2e tests via yarn e2e. I cannot reproduce those issues when I run both main branches.

So, I believe the issue might be in some of the changes to src/app/core in this PR which may be causing side effects on the Browse by Issue Date / Title pages. I could be wrong, but I wanted to report these findings to ensure this PR gets more analysis.

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.

👍 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)

@tdonohue tdonohue added this to the 8.0 milestone Feb 21, 2024
@tdonohue
Copy link
Member

Backend PR is complete. Re-tested this today, and it still works well. Merging immediately.

@tdonohue tdonohue merged commit 45650c1 into DSpace:main Feb 22, 2024
13 checks passed
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 code task high priority improvement new feature
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

UI should have a ProcessPollingService for common polling activities
4 participants