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

feat: Discard experiment #398

Merged
merged 1 commit into from
Feb 7, 2025
Merged

feat: Discard experiment #398

merged 1 commit into from
Feb 7, 2025

Conversation

ayushjain17
Copy link
Collaborator

Problem

No option to discard a created experiment

Solution

Allow discarding of experiment only when it is in created state

Environment variable changes

Needs update in experiment_status_type DB enum

API changes

Endpoint Method Request body Response Body
/experiments/{experiment_id}/discard" PATCH {change_reason: String} ExperimentResponse

@ayushjain17 ayushjain17 requested a review from a team as a code owner January 30, 2025 13:11
@ayushjain17 ayushjain17 force-pushed the exp/discard branch 2 times, most recently from 2ac0601 to 287b0ce Compare January 30, 2025 13:26
.schema_name(&workspace_request.schema_name)
.get_result::<Experiment>(&mut conn)?;

if !experiment.status.is_active()
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about experiment.discardable()?

.await
{
Ok(experiment) => {
log!("experiment response {:?}", experiment);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about a debug log instead?

@@ -450,3 +450,5 @@ VALUES (
null,
'variantIds are used by experimentation module to manage and select variations'
);

ALTER TYPE public.experiment_status_type ADD VALUE 'DISCARDED';
Copy link
Collaborator

Choose a reason for hiding this comment

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

No new line at the of file 😢


view! {
<h3 class="font-bold text-lg">Discard Experiment</h3>
<p class="py-4">Safely discard the experiment without affecting any pre-existing overrides</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Smart sentence

Comment on lines +57 to +65
<textarea
placeholder="Enter a reason for this change"
class="textarea textarea-bordered w-full max-w-md"
value=change_reason.get_untracked()
on:change=move |ev| {
let value = event_target_value(&ev);
set_change_reason.set(value);
}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets use the generic input component for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sauraww is making a generic component for description and reason, it is expected to be used here as well

.await
{
Ok(experiment) => {
log!("experiment response {:?}", experiment);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick:

Suggested change
log!("experiment response {:?}", experiment);
log!("experiment discard response {:?}", experiment);

Comment on lines +81 to +84
let mut redirect_url = format!("admin/{org_id}/{tenant}/default-config");
if let Some(prefix) = key_name {
redirect_url = format!("{redirect_url}?prefix={prefix}")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of mutation redirect_url, you could just overwrite it. Try this sample:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=049ec5fec47ae8a1253bd871838ed69a

@@ -450,3 +450,5 @@ VALUES (
null,
'variantIds are used by experimentation module to manage and select variations'
);

ALTER TYPE public.experiment_status_type ADD VALUE 'DISCARDED';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a failure catch here? This would fail in the future for schemas that already have this Value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why would this fail ?
this is anyways going to used only when a new workspace is going to be created, in which case, the type would not exist at all

@Datron Datron self-requested a review February 7, 2025 12:54
@Datron Datron merged commit 42ac967 into main Feb 7, 2025
4 checks passed
@Datron Datron deleted the exp/discard branch February 7, 2025 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants