-
Notifications
You must be signed in to change notification settings - Fork 67
Add Operator to ValueListParameterIn, ValueListParameterInOut to support passing an operator to a value help
#375
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
base: main
Are you sure you want to change the base?
Conversation
|
CC @d027132 FYI (See also the internal feature request FIORITECHE1-8621.) CC @D041453 @jan-philip-zieher FYI |
HeikoTheissen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You suggest a very broad generalization of the value help concept but give only a narrow use case as justification:
- Valid From shall be on or after some date
- Valid To shall be before or on some date
Do you really need the general case? And don't you need the rule "valid from shall be on or before valid to"? (This seems natural to me, but is not covered by your suggestion.)
vocabularies/Common.xml
Outdated
| </Property> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListParameterIn" BaseType="Common.ValueListParameter"> | ||
| <Property Name="Operator" Type="UI.SelectionRangeOptionType" DefaultValue="0" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of extending the existing type, define a new subtype (suggested name ValueListRangeIn) of ValueListParameterIn and add the new properties to that subtype only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> 2e24e35
vocabularies/Common.xml
Outdated
| <Annotation Term="Core.Description" String="Path to property that is used to filter the value list by applying the operator (Single value or lower interval boundary)" /> | ||
| <Annotation Term="Core.LongDescription" String="In case the property path contains a collection-based navigation or structural property, the filter is a set of `eq` comparisons connected by `or` operators" /> | ||
| </Property> | ||
| <Property Name="LocalDataPropertyHigh" Type="Edm.PropertyPath" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be nullable? For Operator = CP there is no upper boundary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is meant by operator CP, I have only them https://sapui5.hana.ondemand.com/#/api/sap.ui.model.FilterOperator in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the Between use case and the upper boundary with 2e24e35.
| </Property> | ||
| <Property Name="LocalDataPropertyHigh" Type="Edm.PropertyPath" Nullable="false"> | ||
| <Annotation Term="Core.Description" String="Path to property that is used to filter the value list by applying the operator (Upper interval boundary)" /> | ||
| <Annotation Term="Core.LongDescription" String="In case the property path contains a collection-based navigation or structural property, the filter is a set of `eq` comparisons connected by `or` operators" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A collection of values for the upper boundary makes no sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we have here a misunderstanding ? this parameter only needs to be set if a developer wants to default the value to between, although I also don't have a meaningful use case for that, but when using the FE UI you are technically able to configure such scenario, therefore the vocabulary standard should not prohibit us to do so

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the Between use case and the upper boundary with 2e24e35.
vocabularies/Common.xml
Outdated
| </Property> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListParameterInOut" BaseType="Common.ValueListParameter"> | ||
| <Property Name="Operator" Type="UI.SelectionRangeOptionType" DefaultValue="0" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define new subtype ValueListRangeInOut of ValueListParameterInOut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> 2e24e35
|
Hi @PierreFritsch , can you pls. check BLI FIORITECHP1-29107, if it would support you, implementing your use case? I know it's quite old, but I was approached only 2 or 3 weeks ago with question wrt it. So, it looks like someone will take it up, Maybe approach the owner if you are interested. |
@HeikoTheissen thanks for your comments, I pushed how I thought it could be incorporated, please review again. |
HeikoTheissen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First clarify if the requirement is needed in this generality.
- Does the UI support this?
- Are there alternatives (#375 (comment))?
vocabularies/Common.xml
Outdated
| </Property> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListParameterIn" BaseType="Common.ValueListParameter"> | ||
| <Property Name="Operator" Type="Edm.PrimitiveType" DefaultValue="EQ" Nullable="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a concrete type that lists all allowed values (define one if necessary).
vocabularies/Common.xml
Outdated
| <Annotation Term="Core.Description" String="Path to property that is used to filter the value list by applying the operator (Single value or lower interval boundary)" /> | ||
| <Annotation Term="Core.LongDescription" String="In case the property path contains a collection-based navigation or structural property, the filter is a set of `eq` comparisons connected by `or` operators" /> | ||
| </Property> | ||
| <Property Name="LocalDataPropertyBT" Type="Edm.PropertyPath" Nullable="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before, do not clutter the type with additional properties. Define a new subtype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> 2e24e35
Thanks for the hint @BerSie! I had a look at that requirement. The corresponding improvement request is also referenced externally: UI5 Fiori Elements: Semantic Date Range Filter in the SmartFilterBar for Date Interval eg Validity. As the improvement request specifically mentions the filter bar, I'd assume that it wouldn't fit for our use case of prefiltering a value help. |
- Focus on the narrow use case instead of the general case - Use a concrete type `ValueListRangeOperator` that lists all allowed values - Define new subtypes `ValueListRangeIn` and `ValueListRangeInOut` of `ValueListParameterIn` and `ValueListParameterInOut`, and add the new properties to those subtypes only, instead of cluttering the existing types with additional properties.
Thanks for the feedback @HeikoTheissen. Reduced the change to our concrete use case with 2e24e35. |
|
HeikoTheissen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main question is still: Does the UI support the new functionality?
vocabularies/UI.xml
Outdated
| </Member> | ||
| </EnumType> | ||
|
|
||
| <EnumType Name="ValueListRangeOperator"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in UI if it is only used in Common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially placed it here, as it felt like a subset of SelectionRangeOptionType above to me. Moved it to Common (and renamed to ValueListParameterOperator) with 7ff504d.
vocabularies/Common.xml
Outdated
| </Property> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListRangeIn" BaseType="Common.ValueListParameterIn"> | ||
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> | |
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="UI.ValueListRangeOperator/EQ" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the hint! 395a11a
vocabularies/Common.xml
Outdated
| </Annotation> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListRangeInOut" BaseType="Common.ValueListParameterInOut"> | ||
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> | |
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="UI.ValueListRangeOperator/EQ" Nullable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the hint! 395a11a
vocabularies/Common.xml
Outdated
| </ComplexType> | ||
| <ComplexType Name="ValueListRangeIn" BaseType="Common.ValueListParameterIn"> | ||
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> | ||
| <Annotation Term="Core.Description" String="Operator applied to the property." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Annotation Term="Core.Description" String="Operator applied to the property." /> | |
| <Annotation Term="Core.Description" String="Comparison operator used to filter the value list by the property." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vocabularies/Common.xml
Outdated
| </ComplexType> | ||
| <ComplexType Name="ValueListRangeInOut" BaseType="Common.ValueListParameterInOut"> | ||
| <Property Name="Operator" Type="UI.ValueListRangeOperator" DefaultValue="EQ" Nullable="false"> | ||
| <Annotation Term="Core.Description" String="Operator applied to the property." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Annotation Term="Core.Description" String="Operator applied to the property." /> | |
| <Annotation Term="Core.Description" String="Comparison operator used to filter the value list by the property." /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vocabularies/Common.xml
Outdated
| <Annotation Term="Core.Description" String="Initial value, e.g. empty string, is a valid and significant value" /> | ||
| </Property> | ||
| </ComplexType> | ||
| <ComplexType Name="ValueListRangeIn" BaseType="Common.ValueListParameterIn"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subtype is still called "range" but the functional scope is restricted to EQ, LE and GE. Are there plans to extend this in the future to CP and BT? If not, consider
- changing the type name
- whether a subtype is really needed.
One additional property whose default value reflects the current behavior does not necessitate a subtype. I asked for a subtype when it still had many more properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For our current use case, EQ, LE and GE would be sufficient. We currently don't have a need to extend it to CP and BT. Our initial suggestion was meant to match the (SelectionRangeOptionType?) values shown in the value list parameter drop-down.
In the sense of keeping the change simple and covering only our current use case, removed the new ComplexTypes ValueListRangeIn and ValueListRangeInOut with ae2bf15, and moved the Operator to ValueListParameterIn and ValueListParameterInOut instead.
and add the Operator to ValueListParameterIn, ValueListParameterInOut instead
Operator to ValueListParameterIn, ValueListParameterInOut to support passing an operator to a value help
Passing operators to value list parameters is not available in FEv4 yet. The corresponding feature request is tracked internally under FIORITECHE1-8621. Our intention with this PR is to provide support from the vocabulary to move the discussions around the frontend support forward. @HeikoTheissen: What do you think, would it make sense to schedule an alignment call on this requirement with you (the OData team) and with colleagues responsible for prioritizing the FEv4 backlog? We could organize it (CC @jan-philip-zieher 😉). |
We have the requirement of pre-filtering a value help programmatically to preselect valid values for the end users:
Currently, the OData vocabulary for
ValueListParameterInandValueListParameterInOutonly supports theeq(Equal to) operator. Suggesting we extend it so that it also supports at leastle(Less than or equal to) andge(Greater than or equal to).Note: Passing operators to value list parameters is not available in FEv4 yet. The corresponding feature request is tracked internally under FIORITECHE1-8621. Our intention with this PR is to provide support from the vocabulary to move the discussions around the frontend support forward.
Side note: While these two options would be sufficient for our use case, we noticed that the enumeration of selection options contains the operators
bt(Between) andnb(Not between). Supporting all operators would mean that a second value would need to be added to the value list parameters. That would speak for introducingComplexTypes e.g.ValueListRangeIn,ValueListRangeInOutinstead.Hint: This is our first contribution to this repository and that we have a limited capacity for contributing, so feel free to edit/adjust the proposal. Thanks for your comprehension!