Closed (fixed)
Project:
ECA: Event - Condition - Action
Version:
1.0.x-dev
Component:
Code
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Mar 2022 at 16:28 UTC
Updated:
19 Apr 2022 at 13:44 UTC
Jump to comment: Most recent, Most recent file
Show commands
changes
Comments
Comment #2
jurgenhaasThis is now the corrected version:
Comment #4
mxh commentedWe definetly need to cover this with tests so that the mix-up won't happen again.
Comment #5
mxh commentedWe could also create a separate issue that addresses the tests. However I'd recommend to include tests as early as possible, especially on these fundamental ones.
Comment #6
jurgenhaasWorking on tests right here is perfect, I'll get to it now.
Comment #7
jurgenhaasFirst few tests are now available. Please have a look and let me know what else we should be testing.
Note that I'm focusing on testing the condition plugin, not tokens, entities, or field functionality.
Comment #8
mxh commentedThe test is a good start. Some important parts are missing IMO that need to be covered:
I see why you implemented the private evaluate method, but it makes the test harder to read and understand. We should try to write the test as easily readable as possible. It's not a bad thing when instructions are (super) repetitive.
If you need some orientation, a look at the plugin may already give some hints what use cases might occur and should be covered with a test.
Comment #9
jurgenhaas@mxh that sounds as if you only noticed the one test class that tests entity field values. The more comprehensive test is in ScalarComparison which also starts with numeric values, tests the negation, and has a different architecture, which should be even more readable.
Comment #10
mxh commentedAh yes, my bad :) definetly very well readable, especially with the data provider methods, this is great!
Suggestions to
integerDataProvider():Comment #12
jurgenhaasMerged !MR98 for the upcoming beta release but leave the issue open for test completion work.
Comment #13
jurgenhaasTests are now implemented for numeric comparisons as expected with all the different combinations. The negate functionality has only been tested once because it is called for either TRUE or FALSE always in the same way, i.e. if it works once, it always works.
The entity field comparison test has been adjusted to also use a data provider.
In addition, there are now also test for more subclasses like original entity field value, form field value and state value.
The remaining sub-class to compare the routing will get its test in #3271667: Condition "Route match" should return the route name.
The idea behind the tests for StringComparisonBase is that the scalar comparison comes with tests for all the different comparison types and arguments. All other plugins get only tested with fewer tests that foxus on the plugin logic, but don't test all the combinations a second time.
Comment #15
jurgenhaasMR !114 now also includes the tests for RouteMatch condition and TokenLoadRouteParameter action, brought over from #3271667: Condition "Route match" should return the route name
Comment #16
mxh commentedRouteTest is currently not covering access checks. We should test access checks of that action, to make sure that the current user is only able to access allowed route parameter objects.
Comment #17
jurgenhaasI have addressed the comments, but with the access test, I'm having some issues. The route match interface provides parameters without any access test itself. That's why the action plugin doesn't test that either.
Reason being, that the current route will have been access tested before we even get to this point. A current route by definition is accessible by the current user. If it isn't, the route will never be the current one. The route handler will have switched to a 403 or whatever request, before we can even access the current route. Testing, if that works properly, is more a task for the route handler, not for us who are only reading the result.
What might be an interesting scenario though is to test such a request to an inaccessible route, where the route handler has been performing that access control and where our route would no longer be in current route but in parent or master. But I have no idea, how I could get the route interface to do that in a test environment. Suggestions or commits to the MR welcome.
Comment #18
mxh commentedWe are performing an access check on the entity in the "Token: load entity" action and at other places when loading the entity. For consistency, we then also need to perform an access check on the route parameters (I'm not talking about the route object itself). In the MR there is already a test for executing the action for getting a node from the route. The access check of that action should therefore perform an access check on the node that is about to be returned by this action. This may be generalized to check whether the route object to access is an instance of
AccessibleInterface.Comment #19
jurgenhaasIs that a request to improve the test or is this about to change the behaviour of the action?
Comment #20
mxh commentedIn case the action is not yet performing an access check, I'd suggest to adjust the action accordingly, so that the behavior is consistent to the other action implementations of ECA, and cover it within the KernelTest that is already in the MR.
Comment #21
jurgenhaasAgreed, I've now enhanced the access control in the action plugin. I first thought, that's not required because the route handler manages that, but if we access the main route match, then this might be a route where the user doesn't have access.
After that, I also added 2 more tests for this scenario.
Comment #22
mxh commentedReviewed the code, tests are passing, this looks good :)
Comment #24
jurgenhaas:+1: