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

provide NullUser for use when real user is not available #5077

Closed
wants to merge 1 commit into from

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Aug 17, 2021

Primary usage is to provide backward compatibility for methods adding a user: parameter. Whenever possible, a real user should be passed to methods with user: NullUser.new parameter.

Updates usage of user: nil added in PR #5076

@samvera/hyrax-code-reviewers

Primary usage is to provide backward compatibility for methods adding a `user:` parameter.  Whenever possible, a real user should be passed to methods with `user: NullUser.new` parameter.
# @return [nil]
def id
'_NULL_USER_ID_'
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any concerns having a fake id? The tests do not pass if #id returns nil.

ERROR:
Unable to serialize Hyrax::NullUser without an id. raised in ContentUpdateEventJob.perform_later when running the specs for NestedCollectionPersistenceService.

Copy link
Member

Choose a reason for hiding this comment

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

link_to_profile depositor the eventual caller? i think including a broken id is going to (best case) cause a broken link. worse, i think with a real adapter ActiveJob is going to serialize/deserialize a GlobalID, which will fail on the deserialization, preventing the job from running.

i think we might need to consider passing a real user. there are various system users by default. we could use one of those or make a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a suggestion for the 'real' user to pass? This impacts PR #5083 as well. I temporarily used ::User.audit_user for that PR. I am happy to drop the NullUser if there is a viable alternative.

@elrayle elrayle mentioned this pull request Aug 25, 2021
@elrayle
Copy link
Contributor Author

elrayle commented Aug 25, 2021

Replaced by PR #5087 add system user.

@elrayle elrayle closed this Aug 25, 2021
@elrayle elrayle deleted the null_user branch October 27, 2021 18:55
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.

None yet

2 participants