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 actor relation type search, refs #13326 #1106

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

mcantelon
Copy link
Member

Changed how authority record search by relation type works so only the
relation type directly related to an authority record is returned.

@qubot qubot force-pushed the dev/issue-13326-actor-rel-type-search-fix branch from 88a7c56 to b057ebd Compare June 1, 2020 22:28

// Cache result
self::$converseTermIds[$typeId] = $converseTermId;
};
Copy link
Member

Choose a reason for hiding this comment

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

stray ';'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@mcantelon
Copy link
Member Author

Thanks @sbreker ... I've updated the PR.

@@ -54,6 +54,7 @@ static function getEsDocsRelatedToTerm($relatedModelClass, $term, $options = [])

// Allow for options to override default behavior
$search = !empty($options['search']) ? $options['search'] : new arElasticSearchPluginQuery();
$search->query->setSize(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed only to get the count?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed!

@@ -352,6 +354,59 @@ public function serializeAltNames()
return $serialized;
}

public static function serializeObjectDirectRelationTypes($actorId, $relationData)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why public and static?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Made it "protected".

Comment on lines 387 to 393
$relationTerm = QubitTerm::getById($typeId);
$converseTerm = $relationTerm->getConverseActorRelationTerm();

if ($converseTerm !== null)
{
$converseTermId = $converseTerm->id;
}
Copy link
Contributor

@jraddaoui jraddaoui Jun 2, 2020

Choose a reason for hiding this comment

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

It's great that you cached them but I think we should use raw SQL in here, even if they are a few it should be a pretty easy query.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call.

@jraddaoui
Copy link
Contributor

Thanks @mcantelon!

Just a couple of questions and a suggestion, but feel free to disregard.

PS. Somehow this wasn't included in the initial review.

@mcantelon
Copy link
Member Author

Thanks @Radda ! I've updated the PR.

Copy link
Contributor

@jraddaoui jraddaoui left a comment

Choose a reason for hiding this comment

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

Thanks @mcantelon!

@qubot qubot force-pushed the dev/issue-13326-actor-rel-type-search-fix branch from dba4717 to 8516645 Compare June 2, 2020 22:49
Changed how authority record search by relation type works so only the
relation type directly related to an authority record is returned.
@qubot qubot force-pushed the dev/issue-13326-actor-rel-type-search-fix branch from 8516645 to 9128ff8 Compare June 2, 2020 22:56
@qubot qubot merged commit 9128ff8 into qa/2.6.x Jun 2, 2020
@qubot qubot deleted the dev/issue-13326-actor-rel-type-search-fix branch June 2, 2020 22:56
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.

4 participants