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

Access token service doctrine #32

Merged

Conversation

padmasreegade
Copy link

Raising PR to compare. This is not ready for review yet and the code is not fully functional. There are some failing tests that need to be addressed.

Copy link
Owner

@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, @padmasreegade, this is off to a good start, but I spotted a few issues that I think will prevent this from working correctly. I haven't looked closely at the tests yet, since I think it's probably worth fixing the underlying problems first and then seeing how the tests respond. Let me know if you have questions about anything or if I can do more to help, and thanks for all the work on this! I think it's getting close to completion.

module/VuFind/src/VuFind/Db/Entity/AccessToken.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Row/PluginManager.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Service/AccessTokenService.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Service/AccessTokenService.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Service/AccessTokenService.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Service/AccessTokenService.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/Db/Table/PluginManager.php Outdated Show resolved Hide resolved
@padmasreegade padmasreegade requested a review from demiankatz July 31, 2024 12:56
Copy link
Owner

@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, @padmasreegade, this is looking very good. I have just a few very minor suggestions/observations below.

Copy link
Owner

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

@padmasreegade, all tests are passing in my VM, though I have a few minor suggestions below.

Are you still seeing failures on your end? If so, we should figure out what is different between our environments....

Copy link
Owner

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

Looks good, and all tests passing!

@demiankatz demiankatz merged commit 9e0c5c2 into demiankatz:doctrine Sep 4, 2024
3 of 4 checks passed
@padmasreegade padmasreegade deleted the access-token-service-doctrine branch September 4, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants