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

Is the logic used to delete orphaned people correct? #694

Open
CHTJonas opened this issue Jun 27, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@CHTJonas
Copy link
Member

commented Jun 27, 2019

Looking at PeopleRemoveOrphanedCommand#deletePerson I have a small question about the exact logic of the if clause:

if (count($person->getUsers()) == 0 && count($person->getExternalUsers()) == 0) {
    $em->remove($person);
    $output->writeln('Deleted '.$person->getName());
} else {
    $person->setMappedTo(null);
}

Seems that if a person is linked to a user account or an external user then we don't delete that record, instead we unlink it if it's been merged with another person due to eg. misspelling. This is probably a very niche situation but I think we should just delete in all situations here. I tend to lean against having random empty people records even if they are real and claimed by a user.

Triggered by #677.

@GKFX

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

That would mean that all people mapping was deleted I think. After a merge all role records seem to point to the kept person. Having the mapped people provides redirects and history so I would tend to keep them.

@CHTJonas

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

Looks like the initial SELECT query only examines genuine people (that is to say, people who aren't mapped to someone else):

$query = $people_res->createQueryBuilder('p')
    ->leftJoin('p.roles', 'r')
    ->where('p.mapped_to is null')
    ->andWhere('r.id is null')
    ->getQuery();

Basically this if/else behaviour seems totally unrelated to the keeping of mappings for redirects and history, although I may be wrong. In fact, we're actually doing an unnecessary UPDATE here since we're taking people with a null mapto value and then setting that to null again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.