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(apis_entities): only list possible properties in filterset form #959

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

b1rger
Copy link
Contributor

@b1rger b1rger commented Jun 13, 2024

The filter related_properties listed all the existing properties, but
it makes more sense to only list the properties that can have the model
the filter filters as a subj or an obj.

Closes: #949

@b1rger b1rger force-pushed the birger/949-related-property branch from 5c6039c to 4ebe555 Compare June 13, 2024 10:26
@b1rger b1rger marked this pull request as ready for review June 17, 2024 10:26
@b1rger b1rger requested a review from gythaogg June 17, 2024 10:27
@gythaogg
Copy link
Contributor

gythaogg commented Jun 19, 2024

@b1rger In TibSchol I was asked to list the property either as forward or reverse as it applies to the entity type we are searching on.
For example the property composed by/author of between Work and Person
The related property field for Works will contain "composed by" and
the related property field for Persons with contain "author of"

If that makes sense for APIS in general then I'd recommend that as part of #949

When the subject and object are the same type, I displayed the text to show both forward and reverse text to select the property - but in Tibschol they want to see it as two different properties to filter the search better. IMO this is probably just a Tibschol specific requirement - need not be the default behaviour of APIS core.

@b1rger b1rger force-pushed the birger/949-related-property branch 2 times, most recently from 544c32b to b356553 Compare June 19, 2024 11:52
@b1rger
Copy link
Contributor Author

b1rger commented Jun 19, 2024

@b1rger In TibSchol I was asked to list the property either as forward or reverse as it applies to the entity type we are searching on. For example the property composed by/author of between Work and Person The related property field for Works will contain "composed by" and the related property field for Persons with contain "author of"

If that makes sense for APIS in general then I'd recommend that as part of #949

When the subject and object are the same type, I displayed the text to show both forward and reverse text to select the property - but in Tibschol they want to see it as two different properties to filter the search better. IMO this is probably just a Tibschol specific requirement - need not be the default behaviour of APIS core.

It makes sense to display the property with the name based on the entity we are filtering. I've tried to implement that and also added the possible property "targets". I'm not sure if it works as expected, the naming of properties in my test setups is a bit confusing

@b1rger
Copy link
Contributor Author

b1rger commented Jun 19, 2024

(pls. rereview)

@gythaogg
Copy link
Contributor

@b1rger yes - now the property is diplayed as it applies to the entity type as subject.
I created a Character to Place relation - lives in/has as a resident
As expected, in the Character search form I see lives in | place and,
in the Place search form I see has as a resident | character

When the Subject and Object are the same type of entity but have different forward and reverse property labels, such as this Character to Character relationship mother of/daughter of,
I see daughter of | character - which is the name_reverse. It's all right (name_forward would have also been fine), just checking if this the intended behaviour.

This might be because something is wrong with my setup and nothing to do with the pull request - but,

  1. when I submit without selecting any property, I get

    Property matching query does not exist.

  2. when I select a property and then search, I get

    PropertyFilter.filter() got an unexpected keyword argument 'triple_set_from_subj__prop'

@b1rger b1rger force-pushed the birger/949-related-property branch from b356553 to 0a02022 Compare June 24, 2024 05:58
The filter `related_properties` listed all the existing properties, but
it makes more sense to only list the properties that can have the model
the filter filters as a `subj` or an `obj`.

Closes: #949
@b1rger b1rger force-pushed the birger/949-related-property branch from 0a02022 to c615670 Compare June 24, 2024 05:59
@b1rger
Copy link
Contributor Author

b1rger commented Jun 24, 2024

When the Subject and Object are the same type of entity but have different forward and reverse property labels, such as this Character to Character relationship mother of/daughter of, I see daughter of | character - which is the name_reverse. It's all right (name_forward would have also been fine), just checking if this the intended behaviour.

Thats up for discussion. I've now changed that to show name_forward / name_reverse | SubjObjType.

This might be because something is wrong with my setup and nothing to do with the pull request - but,

1. when I submit without selecting any property, I get
   > Property matching query does not exist.

2. when I select a property and then search, I get
   > PropertyFilter.filter() got an unexpected keyword argument 'triple_set_from_subj__prop'

I've fixed that.

Copy link
Contributor

@gythaogg gythaogg left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@b1rger b1rger merged commit a156cb4 into main Jun 26, 2024
11 checks passed
@b1rger b1rger deleted the birger/949-related-property branch July 2, 2024 10:27
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.

search: related property
2 participants