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

IBX-5135: Added LocationDepth and IsMainLocation criterion parsers #50

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

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Feb 20, 2023

Question Answer
JIRA issue IBX-5135
Type improvement/feature
Target version v4.4
BC breaks no
Tests pass yes
Doc needed yes

This PR adds missing parsers for LocationDepth and IsMainLocation criterions.

Example IsMainLocation:

{
  "ViewInput": {
    "identifier": "TitleView",
    "Query": {
      "LocationFilter": {
        "IsMainLocation": 1
      },
      "limit": 10,
      "offset": 0
    }
  }
}

Example LocationDepth:

{
  "ViewInput": {
    "identifier": "TitleView",
    "Query": {
      "LocationFilter": {
        "LocationDepth": {
          "Value": [1],
          "Operator": "eq"
        }
      },
      "limit": 10,
      "offset": 0
    }
  }
}

TODO:

  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@Steveb-p Steveb-p changed the title IBX-5135: Added LocationDepth criterion parser IBX-5135: Added LocationDepth and IsMainLocation criterion parser Feb 21, 2023
@Steveb-p Steveb-p changed the title IBX-5135: Added LocationDepth and IsMainLocation criterion parser IBX-5135: Added LocationDepth and IsMainLocation criterion parsers Feb 21, 2023
@Steveb-p Steveb-p changed the base branch from main to 4.4 February 21, 2023 12:02
@Steveb-p Steveb-p marked this pull request as ready for review February 21, 2023 12:32
@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Steveb-p Steveb-p requested a review from a team February 22, 2023 11:33
@alongosz alongosz requested a review from a team February 22, 2023 12:26
@tomaszszopinski
Copy link

tomaszszopinski commented Mar 1, 2023

Hello, @Steveb-p , please have a look at these queries:

For LocationDepth:

{
    "ViewInput": {
        "identifier": "TitleView",
        "Query": {
            "LocationFilter": {
                "LocationDepth": {
                    "Value": 
                        3
                    ,
                    "Operator": "eq"
                }
            },
            "limit": 300,
            "offset": 0
        }
    }
}

and

{
    "ViewInput": {
        "identifier": "TitleView",
        "Query": {
            "LocationFilter": {
                "LocationDepth": {
                    "Value": 
                        1
                    ,
                    "Operator": "eq"
                }
            },
            "limit": 300,
            "offset": 0
        }
    }
}

For isMainLocation:

{
    "ViewInput": {
        "identifier": "TitleView",
        "Query": {
            "LocationFilter": {
                "IsMainLocation": "asdf"
            }
            ,
            "limit": 30,
            "offset": 0
        }
    }
}

and

{
    "ViewInput": {
        "identifier": "TitleView",
        "Query": {
            "LocationFilter": {
                "IsMainLocation": 1
            }
            ,
            "limit": 30,
            "offset": 0
        }
    }
}

Returned output is the same, seems like criterions dont work as expected.

@tomaszszopinski
Copy link

tomaszszopinski commented Mar 1, 2023

Also, for. the same queries run on elasticSearch im getting no items found. Example :
Input

{
    "ViewInput": {
        "identifier": "title",
        "Query": {
            "LocationFilter": {
                "LocationDepth": {
                    "Value": 
                       2
                    ,
                    "Operator": "eq"
                }
            },
            "limit": 300,
            "offset": 0
        }
    }
}

Output:

{
    "View": {
        "_media-type": "application/vnd.ibexa.api.View+json",
        "_href": "/api/ibexa/v2/views/title",
        "identifier": "title",
        "Query": {
            "_media-type": "application/vnd.ibexa.api.Query+json"
        },
        "Result": {
            "_media-type": "application/vnd.ibexa.api.ViewResult+json",
            "_href": "/api/ibexa/v2/views/title/results",
            "count": 0,
            "time": null,
            "timedOut": false,
            "maxScore": null,
            "searchHits": {
                "searchHit": []
            },
            "aggregations": []
        }
    }
}

@Steveb-p Steveb-p changed the base branch from 4.4 to phpstan August 14, 2023 12:04
@Steveb-p Steveb-p force-pushed the ibx-5135/missing-criterion-parsers branch from 44b47e8 to 87920d7 Compare August 14, 2023 12:04
Base automatically changed from phpstan to 4.5 August 16, 2023 10:58
@Steveb-p Steveb-p force-pushed the ibx-5135/missing-criterion-parsers branch from 0e6f8e2 to 2ad17b3 Compare August 16, 2023 14:05
@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@adamwojs
Copy link
Member

@Steveb-p I guess this PR could back to QA, am I right?

@Steveb-p
Copy link
Contributor Author

@adamwojs we never figured out why exactly there is a discrepancy on the criteria, and the result they give. I suspect they are not working correctly in their base implementation, and REST responses probably are incorrect because of it.

@papcio122
Copy link

Looks like the key 'LocationFilter' from the request is not used anywhere in the code.
Shouldn't 'LocationQuery' be used instead of 'Query' in the request and then just 'Filter' instead of 'LocationFilter'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants