Skip to content

Convert NewItems plugin to service helper#4922

Merged
demiankatz merged 18 commits intovufind-org:dev-12.0from
sambhavp96:remove-newItems-plugin
Dec 8, 2025
Merged

Convert NewItems plugin to service helper#4922
demiankatz merged 18 commits intovufind-org:dev-12.0from
sambhavp96:remove-newItems-plugin

Conversation

@sambhavp96
Copy link
Contributor

No description provided.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@sambhavp96, as you may have already noticed, there appears to be an existing unit test for the NewItems helper -- hopefully you can rename and slightly adjust this to make things pass. I'll give this a closer review once that is done; let me know if I can assist with anything else in the meantime.

@demiankatz demiankatz added this to the 12.0 milestone Dec 2, 2025
@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Dec 2, 2025
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @sambhavp96 -- see below for a few more suggestions!

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for all the progress, @sambhavp96, this looks increasingly good! There are a couple of significant issues with the factory, though. Is the integration test suite passing for you? If so, we may have some gaps that need addressing there, since I would expect the code to be seriously broken by this -- but if you simply haven't had a chance to run the Mink tests yet, I understand! :-)

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @sambhavp96! During final testing of this, I discovered there was a gap in our automated test coverage of new items, so I opened #4947 to address it. This new test revealed some problems related to types, so I made some minor tweaks here to fix them. I also opened #4949 to correct some problematic annotations in existing code.

I am running the full test suite here one more time. Once it passes, I will merge this PR.

@demiankatz demiankatz merged commit fa53830 into vufind-org:dev-12.0 Dec 8, 2025
4 checks passed
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