-
Notifications
You must be signed in to change notification settings - Fork 3
[CDX-496] Add PreFilterExpressionPerSection support to Autocomplete requests #104
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -31,6 +31,17 @@ public class AutocompleteRequest : IFilterable, IUserDetails | |||||
| /// </summary> | ||||||
| public Dictionary<string, Dictionary<string, List<string>>> FiltersPerSection { get; set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets the filter expression used to scope results across all sections. | ||||||
| /// </summary> | ||||||
| public PreFilterExpression PreFilterExpression { get; set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets per-section filter expressions used to scope results for specific sections. | ||||||
| /// Serialized as pre_filter_expression[Section]={...}. | ||||||
| /// </summary> | ||||||
| public List<PreFilterExpressionPerSection> PreFilterExpressionPerSection { get; set; } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets the format options used to refine result groups. | ||||||
| /// </summary> | ||||||
|
|
@@ -105,6 +116,16 @@ public Hashtable GetRequestParameters() | |||||
| parameters.Add(Constants.FILTERS_PER_SECTION, this.FiltersPerSection); | ||||||
| } | ||||||
|
|
||||||
| if (this.PreFilterExpression != null) | ||||||
| { | ||||||
| parameters.Add(Constants.PRE_FILTER_EXPRESSION, this.PreFilterExpression.GetExpression()); | ||||||
| } | ||||||
|
|
||||||
| if (this.PreFilterExpressionPerSection != null && this.PreFilterExpressionPerSection.Count > 0) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we would set together Can we throw here, or at least say "mutually exclusive" in the doc? |
||||||
| { | ||||||
| parameters.Add(Constants.PRE_FILTER_EXPRESSION_PER_SECTION, this.PreFilterExpressionPerSection); | ||||||
| } | ||||||
|
|
||||||
| if (this.TestCells != null) | ||||||
| { | ||||||
| parameters.Add(Constants.TEST_CELLS, this.TestCells); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| namespace Constructorio_NET.Models | ||
| { | ||
| /// <summary> | ||
| /// Associates a <see cref="PreFilterExpression"/> with a specific autocomplete section. | ||
| /// Serialized to the documented bracket-notation shape (e.g. pre_filter_expression[Products]={...}). | ||
| /// </summary> | ||
| public class PreFilterExpressionPerSection | ||
| { | ||
| /// <summary> | ||
| /// Gets or sets the section the expression applies to (e.g. "Products", "Search Suggestions"). | ||
| /// </summary> | ||
| public string Section { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the pre-filter expression to scope results for the section. | ||
| /// </summary> | ||
| public PreFilterExpression Expression { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="PreFilterExpressionPerSection"/> class. | ||
| /// </summary> | ||
| /// <param name="section">Section the expression applies to.</param> | ||
| /// <param name="expression">Pre-filter expression for the section.</param> | ||
| public PreFilterExpressionPerSection(string section, PreFilterExpression expression) | ||
| { | ||
| this.Section = section; | ||
| this.Expression = expression; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="PreFilterExpressionPerSection"/> class. | ||
| /// </summary> | ||
| public PreFilterExpressionPerSection() | ||
| { | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,6 +162,25 @@ protected static string MakeUrl(Hashtable options, List<string> paths, Hashtable | |
| } | ||
| } | ||
|
|
||
| if (queryParams.Contains(Constants.PRE_FILTER_EXPRESSION_PER_SECTION)) | ||
| { | ||
| List<PreFilterExpressionPerSection> preFilterExpressionPerSection = (List<PreFilterExpressionPerSection>)queryParams[Constants.PRE_FILTER_EXPRESSION_PER_SECTION]; | ||
| queryParams.Remove(Constants.PRE_FILTER_EXPRESSION_PER_SECTION); | ||
|
|
||
| foreach (var sectionExpression in preFilterExpressionPerSection) | ||
| { | ||
| if (sectionExpression?.Section == null || sectionExpression.Expression == null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| { | ||
| continue; | ||
| } | ||
|
|
||
| url.Append("&" + Constants.PRE_FILTER_EXPRESSION + UrlEscapedStartSquareBracket) | ||
| .Append(OurEscapeDataString(sectionExpression.Section)) | ||
| .Append(UrlEscapedEndSquareBracket + "=") | ||
| .Append(OurEscapeDataString(sectionExpression.Expression.GetExpression())); | ||
| } | ||
| } | ||
|
|
||
| // Add test cells to query string | ||
| if (queryParams.Contains(Constants.TEST_CELLS)) | ||
| { | ||
|
|
||
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.
It passes even if we emit the JSON unescaped
Also, there is one `Assert. ', it hides which part failed, maybe we can make a few asserts as the fmt_options test above?