Skip to content

Refactor tag administration to use database services#3703

Merged
demiankatz merged 14 commits intovufind-org:devfrom
demiankatz:tag-admin-refactor
Jun 6, 2024
Merged

Refactor tag administration to use database services#3703
demiankatz merged 14 commits intovufind-org:devfrom
demiankatz:tag-admin-refactor

Conversation

@demiankatz
Copy link
Member

@demiankatz demiankatz commented May 25, 2024

This PR backports (and slightly improves/modifies) the tag administration port from #2233. The admin TagsController now exclusively uses database services instead of Laminas table/row classes.

One slightly controversial decision may be the use of a \Laminas\Paginator\Paginator as a return value from one of the ResourceTagsService methods, but since adapters exist for both Doctrine and Laminas database code into Laminas paginators, this seemed like a reasonably forward-compatible solution for pagination, at least for the moment.

It would also be a good idea to add Mink test coverage for this functionality, but I unfortunately do not currently have the available time. If time permits, I might work on that while I'm merging this into #2233.

@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label May 25, 2024
@demiankatz demiankatz added this to the 10.0 milestone May 25, 2024
@demiankatz demiankatz requested review from EreMaijala and aleksip May 25, 2024 00:29
@demiankatz demiankatz removed the request for review from EreMaijala May 30, 2024 13:41
@demiankatz
Copy link
Member Author

I've also pushed up Mink test coverage of the admin functionality to make this easier to test once I've merged it into #2233... once the full test suite passes in my test environment, I will merge this.

@demiankatz demiankatz requested a review from aleksip May 30, 2024 14:37
@demiankatz
Copy link
Member Author

demiankatz commented May 30, 2024

@aleksip, running the full test suite actually revealed a long-standing problem that had not been noticed before: the admin tools were not cleaning up orphaned tags from the tags table; they were only deleting links. I've added a new database service method to clean this up, and integrated it with the admin tool. I thought about the possibility of a more automatic orphan cleanup tied in with other existing methods, but doing it more explicitly and in a granular fashion ultimately felt safer and clearer.

However, since I've made a pretty significant change with this, I thought it best to re-request review before merging in case you have any thoughts about this.

@demiankatz
Copy link
Member Author

And just to confirm: the full test suite is now passing (it was failing before due to unexpected tags in the database during a later test). So as long as my new changes look okay, this should be all clear to merge.

Copy link
Contributor

@aleksip aleksip left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry this took some time for me to review, I wanted to run the full tests and setting up a working test environment turned out to be more work than I expected.

@demiankatz demiankatz merged commit f8308ec into vufind-org:dev Jun 6, 2024
@demiankatz demiankatz deleted the tag-admin-refactor branch June 6, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

architecture pull requests that involve significant refactoring / architectural changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants