Add options for Query Based Parameter for a query with parameters

[gabrieldutra]

What type of PR is this? (check all applicable)

• Feature

Description

This PRs allows selecting a Parameterized Query for Query Based Parameters by mapping its parameters as Dropdown Search or Static value.

Related Tickets & Documents

#3014

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[gabrieldutra]

(From @arikfr 's comment #3014 (comment))

Static value option has its own complexity, specially when considering backend validation for values. So if it makes implementation more complex, we can skip it as well, and only show a message that saying: "Only single parameter can be used" or something like this. And later implement the new functionality.

After my first iterations with the backend I wouldn't say the static value has its complexity for validation rules. Since they are configured in the Parameter Edit, the values should be reliable (it's just a matter of computing them backend-wise on the validation).

However, a bigger problem in the same manner is the Search parameter. At a moment I tried to handle the safety of Parameters with the real time Search enabled, but realized it falls back on being a Text Parameter (just with one more layer before it 😅). I'm not sure if you already had that in mind before, but I guess those will have to be a new entry on the "Unsafe param list".

Based on this, I got a few options on how to proceed:

1. Make only Dropdowns with the Search feature enabled unsafe: more robust and allow not that many more cases;
2. Make all Dropdowns based on any query parameters unsafe: this safes us from digging into query results to validate such parameters. I already have the piece of code that evaluates this in this PR from my tests, but I think it's still a pain to have it running on every query execution.

Personally I prefer going with 2 as that logic only made sense to me for the Search feature. Anyway, I'm sharing this in case you had other thoughts on this and to make sure I'll go into the right direction :)

[arikfr]

It's an overlook on my part :-( Unfortunately you're right: for the original use case of a search parameter which is text based, we will have to mark those as unsafe.

While option 1 is better, option 2 is also acceptable for first iteration as a text based parameter is the most common case anyway.

[gabrieldutra]

Should this be used in other get_by_id_and_org uses too?

[gabrieldutra]

I guess the implementation here is pretty much done, so I'll brief it.

UI
Basically the same as the Dashboard Parameters mapping (this should cover adding the Propagate to parent query option in the future).

The Parameter Mapping is a required in the Form.

Functionality
I chose my option 2 to go with to have most part of the functionality in the frontend and avoid replicating code on the backend. So, validation is disabled for Parameterized Query based parameters and they are marked as unsafe (otherwise would need to rerun the query on the backend to confirm the dropdown values).

For the parameter value in the URL it was necessary to add the search term since the value is now always coupled to it. I added it as p_value={searchTerm}--{paramValue}, lmk if you have better ideas.

Next steps

• Add backend unit tests for this PR changes
• Add Cypress tests for the UI (perhaps in a following PR)
• Update messages about unsafe parameters (most of them just mention "text parameters")

[rauchy]

Wouldn't we prefer that this blows up with a 404 in such a case? Returning False implies that this query doesn't have parameters, but in-fact, it's a misconfigured parent query. It might make debugging harder.

[gabrieldutra]

Returning False was just the easy option (because it will invalidate any option you provide to the parameter), I actually just wanted to avoid the error happening on the server and check what should be done in the rest of this file (currently a server error happens for misconfigured parent queries due to _load_results too).

Rethinking about it, to better cover the misconfigured parameters at this point we can either create a new exception type or reuse InvalidParameterError and blow up with a 400. IMO there's probably a more robust way, but showing "Your query has misconfigured parameterers: {}" error on query execution should be helpful enough.

[gabrieldutra]

I now wonder if it's worth it to add the extra logic to have a new Exception type, considering that #4707 will show no options and add a "No options available" empty state. Also I think any extra work here may end up on rework from #4312

[gabrieldutra]

Updated it to consider that NoResultFound as an invalid parameter, the UI will still show it as required when editing. This should be enough help for identifying a problem with a query based dropdown.

[rauchy]

I'm not too happy about stubbing out implementation details such as query_has_parameters. Is there a way to setup the data to organically provide this behavior?

[gabrieldutra]

Unfortunately my experience with the backend unit tests is very limited 😅, for this one I just cloned the behavior it had for load_results since it was returning a "There is no app context".

Another option I have in mind is getting rid of that method and creating a get_query_by_id_and_org (so the stub would be a query, not a boolean)

[gabrieldutra]

A few months later and I'm a little bit better at this now 🙂. So, turns out the test_parameterized_query is based in TestCase, thus it doesn't have an App Context, we then can:

1. Make it based in BaseTestCase, so that it will have an App Context for us to create an actual query. Downside: will make it slow;
2. Keep it like that, add a _load_dropdown_query that we can mock and keep mocking dropdown_values with a fast running test;

Any preference in this case?

[gabrieldutra]

This test was actually harder to set with the organic App Context than I thought, so I'm back with the mocks, but I've updated the query_has_parameters to be a mock on the Query object.

[gabrieldutra]

I realized that although way it works with making options coupled to a search term is better in terms of validating the options, this will cause UX issues 😕. So I'll take a stab at storing both Value and Label (both as the query default value and in the search url).

[jhult]

Looking forward to this.

[justinclift]

@gabrieldutra This PR sounds useful. Should we leave this open?

If you don't have time to look at it though, we can just close it. 😄

[konnectr]

I see this as an important feature )

[justinclift]

@konnectr Cool. 😄

Any interest in trying to finish the PR?