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

nullpointer fix when getting notification with newly created user #8969

Merged
merged 5 commits into from
Nov 10, 2022

Conversation

ErykKul
Copy link
Collaborator

@ErykKul ErykKul commented Sep 9, 2022

What this PR does / why we need it:
This fixes the nullpointer on DataverseUserPage when creating a new user.

@coveralls
Copy link

coveralls commented Sep 9, 2022

Coverage Status

Coverage decreased (-0.0003%) to 19.98% when pulling e16c886 on ErykKul:notifications_nullpointer into 11abccf on IQSS:develop.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Sep 9, 2022

After some testing: when a user is created with shibboleth in UI, that user is directly set in the session. It gives nullpointer until you log out and log in again (user is loaded from database). This pull request should fix that.

@pdurbin
Copy link
Member

pdurbin commented Oct 5, 2022

I moved this to review because just now @donsizemore logged into Harvard Dataverse with a new Shib account and got a 500 error when he clicked My Data.

As far as we understand, the workaround (until this PR is merged) is to log out and back in. That way the "muted emails" code checks the database and is initialized.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Oct 6, 2022

This workaround is valid. Technically, it is a little bit different: with log-out and log-in again nothing is written in DB, but the object is initialized differently (muted e-mails are empty i.s.o. being null). This pull request fixes the initialization of the object when it is constructed by creating a new account (it was the case I had missed when making the initial pull request). Until you mute something, nothing is written in DB.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Another proposed fix just came in:

@ErykKul since you wrote the original code, I'm inclined to go with your fix. 😄 Any thoughts on the other proposal? (If you want you could leave a review on it.)

@mreekie
Copy link

mreekie commented Oct 12, 2022

Phil points out that testing of this will not be straightforward based on our current environment.
The test server will need to be setup.
This will impact the size of this work to QA this.

To do this the right way - will require a full Shiboleth setup.
The shorter way is to do it in the artificial dev mode.

This calls out that we do not have a current shiboleth environment.

For this we'll do this with the artificial dev mode for this PR.

@sekmiller sekmiller self-assigned this Oct 13, 2022
@scolapasta scolapasta assigned scolapasta and ErykKul and unassigned sekmiller Oct 31, 2022
@scolapasta scolapasta assigned landreev and unassigned scolapasta and ErykKul Nov 7, 2022
@landreev landreev merged commit 32ced00 into IQSS:develop Nov 10, 2022
@pdurbin pdurbin added this to the 5.13 milestone Nov 21, 2022
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.

7 participants