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

Update Experiment Assignment Query form to use EditSqlModal. #1370

Merged
merged 23 commits into from Jul 10, 2023

Conversation

mknowlton89
Copy link
Collaborator

@mknowlton89 mknowlton89 commented Jun 13, 2023

Features and Changes

This PR updates the AddEditExperimentAssignmentQueryModal to consume the new EditSqlModal that was introduced when we added support for the SchemaBrowser. Initially, when we launched the SchemaBrowser we didn't add support for it within the AddEditExperimentAssignmentQueryModal as that modal had a few features unique to it - such as support for hasNameCol and dimension columns.

Previously, when building an experiment assignment query, the user would check the box if they would like to useNameCols and they would manually enter what dimension columns they wanted to include in the query. The SqlInputField then took the value of those features into account when determining the requiredColumns.

Now, we've introduced an "Advanced" section that is collapsed by default where the user can enable useNameCols and add dimensions. The existing "Test Query" logic is still in place - with this, if a user tests a query that contains both name columns, we will automatically enable useNameCols for them (the opposite is also true - if they have useNameCols enabled, but the query doesn't contain either column, we'll automatically disable useNameCols for them. Likewise, if a user tests a query that has columns outside of the required columns, we'll automatically add that as a dimension column when they use the "Test Query" button, or if they save the form with "Test query before saving" checked.

When it comes to dimension columns, the logic mirrors the above - if a dimension is included in the query, we'll automatically add it to the dimensions array, and the opposite is also true, if the user has entered a dimension into the dimensions array in the "advanced" section, but they don't include it in the query, rather than throwing an error, we just remove it from the dimensions array.

The only time we'll throw an error for a missing column is if the column isn't a dimension.

This feels little weird, but it is inline with the logic around useNameColumns and the UX if we don't do this is kinda weird. (E.X. We'd throw an error that they're missing a dimension, and they'd have to close the EditSqlModal -> remove the dimension from the dimensions input in the advanced section, and then open the EditSqlModal again.

I've also added a warning message if the query isn't changed from the default sql value so users know they need to customize it.

And finally, this PR includes a checkbox within the EditSqlModal to Test Query Before Saving - when checked, we'll test the query and if there are any errors, we'll surface those and block saving/closing the modal.

Before

Screen Shot 2023-05-04 at 11 29 48 AM

After

Screen Shot 2023-07-06 at 6 41 08 AM

Testing

  • Create a new experiment assignment query, and write a valid sql query that does not include any additional columns and doesn't not include experiment_name and variation_name columns and ensure the query is saved successfully, and within Mongo the query object's hasNameCol property is false, and the dimensions array is empty.

  • Create a new experiment assignment query, and write a valid sql query that DOES include an additional column (e.g. "browser"), but doesn't not include experiment_name and variation_name columns and ensure the query is saved successfully, and within Mongo the query object's hasNameCol property is false, and the dimensions array is empty.

  • Create a new experiment assignment query, and write a valid sql query that DOES include an additional column (e.g. "browser"), but doesn't not include experiment_name and variation_name columns and when clicking the Test Query the extra column is added as a dimension automatically.

  • Create a new experiment assignment query, and write a valid sql query that DOES include an additional column (e.g. "browser"), and DOES include experiment_name and variation_name columns and that when clicking the Test Query button, the userEnteredHasNameCols is flipped to true.

  • Create a new experiment assignment query and write an invalid sql query, and ensure that clicking "Save" throws an error and blocks the user from being able to save the query. Now, uncheck the "Test before saving" box and click save again and ensure the form is closed and the query is saved, despite being invalid.

  • Create a new experiment assignment query and write an invalid sql query and click the "Test Query" button, ensure that an error message is displayed - now, fix the query, and click the "Save" button with the "Test before saving" box checked, and ensure the form is closed and the query is saved successfully.

  • Create a new experiment assignment query and don't toggle the "useNameCols" automatically, but instead write a query that is valid, and add experiment_name as a column and click the "Save" button with "Test before saving" box checked - ensure that an error is displayed telling the user to either remove experiment_name or add variation_name - Add variation name and click "Save" again and ensure the form closes and the query is saved. And ensure useNameCols has been automatically enabled.

  • Now, open the same exp assignment query and remove both experiment_name and variation_name columns and click "Save" with "Test before saving" checked, and ensure it saves correctly, and useNameCols is now disabled in the "Advanced" section

  • Create a new experiment assignment query and write an valid sql query and click the "Test Query" button, ensure that rows are returned, now, remove a required column that ISN'T a dimension, and ensure you get an error message. Add that required column back in, and this time, add a dimension and click "Test Query" and ensure that the requiredColumns array is updated with the new dimension. And, lastly, remove the new dimension from the query and click "Save" with "Test query before saving" - Ensure it saves correctly, and the dimension has been removed successfully.

When we're rendering the modal, we're passing the error in via the error prop

<Modal
      open
      header="Edit SQL"
      submit={form.handleSubmit(async (value) => {
        if (testQueryBeforeSaving) {
          // Reset test query results to avoid serving outdated errors/results
          setTestQueryResults(null);
          const res = await runTestQuery(value.sql);
          if (res.error) {
            throw new Error(res.error);
          }
        }

        await save(value.sql);
      })}
      close={close}
// THIS IS WHERE WE'RE PASSING THE ERROR IN - WE'RE CALLING setTestQueryResults(null) which should reset it, but it's not
      error={testQueryResults?.error}
      size="max"
      bodyClassName="p-0"
      cta="Save"
      closeCta="Back"
      secondaryCTA={
        <label>
          <input
            type="checkbox"
            className="form-check-input"
            checked={testQueryBeforeSaving}
            onChange={(e) => setTestQueryBeforeSaving(e.target.checked)}
          />
          Test Query Before Saving
        </label>
      }
    >

Closes GB-78

@github-actions
Copy link

github-actions bot commented Jun 13, 2023

Your preview environment pr-1370-bttf has been deployed.

Preview environment endpoints are available at:

…ers know they need to customize it. Also simplifies the logic for showing the advanced mmode div.
@linear
Copy link

linear bot commented Jun 16, 2023

GB-78 (Frontend) - Add component to experiment assignment builder

The AddEditExperimentAssignmentQuery modal has some additional logic that we need to mirror around "Named Columns". If the toggle is selected, we

@mknowlton89 mknowlton89 self-assigned this Jun 16, 2023
@mknowlton89

This comment was marked as outdated.

@mknowlton89

This comment was marked as outdated.

await save(value.sql);
})}
close={close}
error={testQueryResults?.error}

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@mknowlton89 mknowlton89 requested a review from jdorn July 6, 2023 15:27
@mknowlton89 mknowlton89 marked this pull request as ready for review July 6, 2023 15:28
Copy link
Collaborator

@bttf bttf left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the demo! Just one minor suggestion

save={async (userEnteredQuery) => {
form.setValue("query", userEnteredQuery);
}}
validateResponse={(result) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's probably worth just defining this as a function outside of the component body ... just for readability

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

10-4! Just updated that in this commit - thanks for your review!

@mknowlton89 mknowlton89 merged commit a09f0b0 into main Jul 10, 2023
4 checks passed
@mknowlton89 mknowlton89 deleted the mk/exp-assign-query branch July 10, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants