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

Fix Item Counts (webui.strengths) by refactoring ItemCounter and ItemCountDAO to act like other Spring beans #9583

Merged
merged 8 commits into from
May 24, 2024

Conversation

tdonohue
Copy link
Member

@tdonohue tdonohue commented May 15, 2024

References

Description

This PR refactors the ItemCounter and ItemCountDAO to be Spring beans defined in core-services.xml. This fixes issues with Item Counts not reloading & issues where it was not possible to use live, database-based counts (via webui.strengths.cache=false).

This refactor involved the following:

  • Define both ItemCounter and ItemCountDAO as beans in core-services.xml
    • Remove ItemCountDAO configuration from dspace.cfg as it is now configured via core-services.xml
  • Remove caching of Context object in ItemCountDAO. This resulted in database connection errors when the Context was outdated
  • Remove duplicative countArchivedItems() methods from Community and Collection as these were just wrappers around the same methods in CommunityService and CollectionService.
  • Update code to latest standards (minor code cleanup & autowiring of all necessary service classes)
  • Remove unnecessary ItemCountDAOFactory and ItemCountException (the exception handling was updated to use log.error as otherwise item counting exceptions would crash the UI).

Instructions for Reviewers

  • Deploy PR
  • Ensure Item Counts (webui.strengths): Cached Counts Not Updating  #9434 is fixed by doing the following:
    • Set webui.strengths.show = true and webui.strengths.cache=true on the backend. This tells the backend to use Solr for all Item Counts
    • In the UI, Look at the current item count for any Collection and its parent Community(ies)
    • Submit a new Item to that Collection.
    • Once item is archived, ensure the Collection item count & parent Community item counts increase by one. This should happen almost immediately, but a reload of the page may be required.
  • Ensure Item Counts (webui.strengths): Live Counts Break the Community List #9437 is fixed by doing the following:
    • Set webui.strengths.show = true and webui.strengths.cache=false on the backend. This tells the backend to use the Database for all Item counts.
    • In the UI, ensure Item Counts on homepage & Community/Collection lists all still work & are accurate. (Currently, on main and dspace-7_x they will throw errors as described in the ticket)

… Also ensure they do not cache a Context object.
@tdonohue tdonohue added bug high priority component: Discovery Related to Discovery search or browse system 1 APPROVAL pull request only requires a single approval to merge. testathon Reported by a tester during Community Testathon port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels May 15, 2024
@tdonohue tdonohue added this to the 8.0 milestone May 15, 2024
@tdonohue tdonohue requested a review from michdyk May 16, 2024 14:23
@damian-joz
Copy link
Contributor

Hi @tdonohue, I've started testing phase. I've noticed that when item is withdrawn, the counter still include it. I've done some small fix for that. Can I push it directly to your branch?

@tdonohue
Copy link
Member Author

@damian-joz : If GitHub allows you to push to my branch, then please feel free. If it doesn't allow you, then you could send me a PR to my branch (by creating at PR against my GitHub account: https://github.com/tdonohue/DSpace), or send me the code.

Thanks for testing & for finding a fix to the issue you discovered!

…em model (discoverable, archived, withdrawn)
@damian-joz
Copy link
Contributor

damian-joz commented May 24, 2024

@tdonohue I've created a PR to your branch. I've also tested a few scenarios:

  • adding a new item
  • withtdrawing an item
  • making an item non-discoverable
  • moving item from one collection to another
  • deleting item
  • mapping item to another collection,

both for cache(Solr) and non cache solution(Database query) and it works fine. Sometimes it required the refresh of the UI.

Just one thing worth to mention, when we will map item to another collection we end up with something like this:
image. And for some users it can be confusing because community counter is not even with the sum of collection counters. However it is ok because Collection 1 has 3 items with one item shared between collections.

@damian-joz damian-joz self-requested a review May 24, 2024 14:12
@michdyk michdyk removed their request for review May 24, 2024 14:13
Refactor item counter fixes after review
@tdonohue
Copy link
Member Author

I've merged the improvements that @damian-joz made in tdonohue#15 to this branch. Those are now included in this PR.

As @damian-joz has given approval (thanks!), I'll give this another test on my end to make sure everything still works well (after this most recent update). Assuming everything is working well, I'll get this merged for 8.0 and backported to 7.x

Copy link
Member Author

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

Retested this again today and verified that @damian-joz 's changes work well. Non-discoverable items are no longer included in counts. Additionally, withdrawn items are no longer included in counts. When withdrawing an Item or making it undiscoverable, the counts for parent collections/communities update almost immediately.

So, I think this all looks good! Merging for 8.0 and backporting to 7.x

@tdonohue tdonohue merged commit b02a7f9 into DSpace:main May 24, 2024
22 checks passed
@dspace-bot
Copy link
Contributor

Backport failed for dspace-7_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-7_x
git worktree add -d .worktree/backport-9583-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-9583-to-dspace-7_x
git switch --create backport-9583-to-dspace-7_x
git cherry-pick -x 78f1e4190e0f48bc707416968033578d6ce12b9c d07aab6025a4e23be767146e03bc1964169c8a30 a4c297c94728d17081260e60cb0bc498d7f92a8d 01be5eee419b9d79192ed55a927a4b271a094bb0

@tdonohue tdonohue deleted the refactor_item_counter branch May 24, 2024 21:41
@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label May 31, 2024
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. bug component: Discovery Related to Discovery search or browse system high priority testathon Reported by a tester during Community Testathon
Projects
No open projects
Status: ✅ Done
3 participants