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

add system user #5087

Merged
merged 2 commits into from
Aug 25, 2021
Merged

add system user #5087

merged 2 commits into from
Aug 25, 2021

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Aug 25, 2021

This user is logged as the acting user for jobs and other processes that run without being attributed to a specific user (e.g. creation of the default admin set).

Used by PR #5083, update AdminSetCreateService to create valkyrie resources, when creating the default admin set.

Replaces PR #5077 which looked at adding a fake NullUser. It was decided that a real user was expected.

@samvera/hyrax-code-reviewers

This user is logged as the acting user for jobs and other processes that run without being attributed to a specific user (e.g. creation of the default admin set).
@@ -145,6 +145,15 @@ def all_user_activity(since = DateTime.current.to_i - 1.day)
end

module ClassMethods
# Override this method if you aren't using email/password
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this mean? will this addition be breaking for users who "aren't using email/password"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this comment from audit_user. The same comment exists for batch_user. I assumed it would apply for system_user as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know if it would be a breaking change. I suspect not as there is a default configuration for the system_user_key. But I'd have to see what an alternative implementation looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git blame says the comment for audit was added 9 years ago

@no-reply
Copy link
Contributor

no-reply commented Aug 25, 2021

looks good. i have a question about the comment. i'm wondering whether it's accurate/helpful.

i get that it's copied from below, but for my part, i don't understand what it means. i wonder whether we expect application implementer to see it, and how they would know what override is needed if they did.

The override comment is not clear and is being removed.
@no-reply no-reply merged commit 3162a7e into main Aug 25, 2021
@no-reply no-reply deleted the system_user branch August 25, 2021 20:14
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