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

fix: incorrect created date shown on backup method #572

Open
wants to merge 4 commits into
base: 5
Choose a base branch
from

Conversation

gavynj
Copy link

@gavynj gavynj commented Oct 18, 2024

This PR fixes issue #571

When viewing a member, at the bottom of their member screen on the backend it says whether they have MFA setup and when their recovery codes were created. However, for any user that has MFA setup, the recovery codes created date always shows as today.

@michalkleiner
Copy link
Contributor

Nice finds! The changes look ok to me. I wonder whether we should require the member to be passed always and not fallback to the current user at all.

You may also want to update the doc block when you add a method param and strongly type it to Member.

@michalkleiner
Copy link
Contributor

The lint failure is related.

@gavynj gavynj requested a review from GuySartorelli October 31, 2024 22:03
Comment on lines 51 to 55
if (!$this->value && $this->getForm() && $this->getForm()->getRecord() instanceof Member) {
$member = $this->getForm()->getRecord();
$this->member = $this->getForm()->getRecord();
} else {
$member = DataObject::get_by_id(Member::class, $this->value);
$this->member = DataObject::get_by_id(Member::class, $this->value);
}
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I think it would be better to convert this part into a private method that returns the member record, and can be called from both getSchemaDataDefaults() and getBackupMethod().

That way we never have to fall back on ?? Security::getCurrentUser() if getBackupMethod() is called first.

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.

3 participants