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 handle conditions #173

Closed
wants to merge 5 commits into from
Closed

Fix handle conditions #173

wants to merge 5 commits into from

Conversation

bakridemos
Copy link

@bakridemos bakridemos commented Aug 19, 2024

Ticket: https://demoseurope.youtrack.cloud/issue/DPLAN-12254/Filter-Bearbeiter-Nicht-zugewiesen-funktioniert-nicht

Description: in some cases however conditions do have values, we do need the OPERATORS 'IS NULL' and 'IS NOT NULL' and since they are only available in the condition without values, some filters are not working anymore

…s, we do need the OPERATORS 'IS NULL' and 'IS NOT NULL' and since they are only available in the condition without values, some filters are not working anymore
…s, we do need the OPERATORS 'IS NULL' and 'IS NOT NULL' and since they are only available in the condition without values, some filters are not working anymore
…s, we do need the OPERATORS 'IS NULL' and 'IS NOT NULL' and since they are only available in the condition without values, some filters are not working anymore
@bakridemos bakridemos added the bug Something isn't working label Aug 19, 2024
@bakridemos bakridemos self-assigned this Aug 19, 2024
@bakridemos bakridemos marked this pull request as ready for review August 19, 2024 10:27
Comment on lines +54 to +60
if (array_key_exists(DrupalFilterParser::VALUE, $condition )) {
if (!array_key_exists(DrupalFilterParser::OPERATOR, $condition )
|| ($condition[DrupalFilterParser::OPERATOR] !== 'IS NULL'
&& $condition[DrupalFilterParser::OPERATOR] !== 'IS NOT NULL')) {
return $this->drupalConditionFactory->createConditionWithValue($operatorName, $condition[DrupalFilterParser::VALUE], $path);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handling specific operators is not the responsibility of this class. You can pass any implementation of DrupalConditionFactoryInterface into this class' constructor to support any operator with any value as you desire.

Such implementation can be done outside of this library as it depends on the specific use case and filters you want to support, which should not be coupled to this library.

Choose a reason for hiding this comment

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

just some information about why this came up...
image
We can fix this issue outside of EDT. This was still a BC introduced by splitting this method so maybee it is not intended.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you intend to handle the value unassigned in this example?

Choose a reason for hiding this comment

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

There is no need for this value 'unassigned' appart from informing the FE about there filterChoice and we tried removing it.
If the value is not set - we get a constraint violation within the drupalFilterValidator that complains about a missing Value. But that means the condition to call the method createConditionWithoutValue cant be met.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the value is not set - we get a constraint violation within the drupalFilterValidator

@MoritzMandler: In that case I'd like to verify the correct behavior of DrupalFilterValidator. From what you wrote I deduced the following test case. It succeeds in my environment, though I'm not sure regarding the exact branch you're on. Could you please adjust it based on your actual observations, so that the mentioned constraint violation is thrown?

$factory = new PredefinedDrupalConditionFactory(
    new DqlConditionFactory()
);
$validator = new DrupalFilterValidator(
    Validation::createValidator(),
    $factory
);
$validator->validateFilter([
    "foo" => [
        "condition" => [
            "path" => "assignee",
            "operator" => "IS NULL"
        ]
    ]
]);

Choose a reason for hiding this comment

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

Thanks a lot for your help.
We found out that the request was falsely structured after removing the value 'unassigned'.
By fixing it everything works fine without the need to change the EDT stock behavior.
Here was the wrong format that gave us these headaches...
image

@bakridemos
Copy link
Author

this changes are not required anymore, this will be handled in core

@bakridemos bakridemos closed this Sep 3, 2024
@bakridemos bakridemos deleted the fix-handle-conditions branch September 3, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants