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

Default required-string User getters to blank strings if unset. #3657

Merged
merged 2 commits into from
May 13, 2024

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented May 13, 2024

While these values are always set in the database, we need these defaults for PrivateUser support.

(This fixes a bug and test failure introduced by #3639).

- While this value is always set in the database, we need this default for PrivateUser support.
@demiankatz demiankatz added this to the 10.0 milestone May 13, 2024
@demiankatz demiankatz requested a review from ThoWagen May 13, 2024 13:50
@demiankatz
Copy link
Member Author

demiankatz commented May 13, 2024

@ThoWagen, I discovered after merging #3639 that it introduced a test failure. I suspect this was not caught sooner because the test in question was introduced close to the time #3639 was opened and may not have been merged to dev yet when I ran tests earlier. In any case, this minor adjustment should get everything under control again -- the issue is that when we store user data in the session instead of the database for privacy mode, there is a chance of null values in unexpected places, and this caused an inappropriate null return from a string-typed method.

Copy link
Contributor

@ThoWagen ThoWagen left a comment

Choose a reason for hiding this comment

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

I'm wondering if the other getter methods should also return defaults or otherwise throw a custom exception if the value is accessed before being set.

@demiankatz
Copy link
Member Author

@ThoWagen, I had the same thought. Only question is whether to do it here, or to merge this to fix the broken build and then do a more thorough job as a follow-up. Either way, I'll start looking into changes now...

Copy link
Contributor

@ThoWagen ThoWagen left a comment

Choose a reason for hiding this comment

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

Alright. Then I approve this to quickly fix the bug.

@demiankatz demiankatz changed the title Default last language to blank string if unset. Default required-string User getters to blank strings if unset. May 13, 2024
@demiankatz
Copy link
Member Author

@ThoWagen, I built #3658 to create a simple way to call all the getters and see what breaks. That revealed a couple more fields in need of fallbacks, which I have added here.

@demiankatz demiankatz merged commit ed236ee into vufind-org:dev May 13, 2024
7 checks passed
@demiankatz demiankatz deleted the fix-getlastlanguage branch May 13, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants