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

PE-643 Add a weekly job to sync user status and info from Entra ID #623

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kxwang
Copy link
Contributor

@kxwang kxwang commented Oct 31, 2024

No description provided.

return;
}

// Get the day of the week to sync. Default is Saturday.
$sync_day = \Drupal::state()->get('epgov.user_sync.day_of_week') ?? "Saturday";
if(date("l") != $sync_day) return;
Copy link
Member

Choose a reason for hiding this comment

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

I think the epgov.user_sync.interval_in_days setting is potentially misleading/confusing since it will only ever run on the set day of week. For example, setting the interval_in_days to 2 won't make it run every other day; it will still only run on Saturdays. So it's not clear to me how/why someone would use the interval_in_days setting.

if( $queue != null ) $queue->deleteQueue();

// Set the Day of Week to today
\Drupal::state()->set('epgov.user_sync.day_of_week', date("l"));
Copy link
Member

Choose a reason for hiding this comment

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

Won't this change all future user sync cron runs from Saturday to whatever day this drush command was run on>


// Look up user by Drupal user name (Principal name in AD)
// Sometimes a user will be recreated with the same principal name but different AD ID
$users = \Drupal::entityTypeManager()->getStorage('user')->loadByProperties(['name' => $user_data['userPrincipalName']]);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this use the trimmed principal name in case the name was trimmed when the user was created (line 109)?

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