Remove update environments from putOrganization. #2564

mknowlton89 · 2024-05-23T16:52:35Z

Features and Changes

This PR updates how we handle CRUD actions for an organization's environments. Previously most of this logic was nestled within the putOrganization method, and we're making an effort to reduce the overall power of that method and instead, handle the logic in specific controllers.

This PR removes the ability for an organization to update their environments via this method, and instead, moves it to the environments.controller.ts file.

This PR introduces/moves the following logic:

Adds specific delete route
Adds specific put/:id route
Adds specific updateOrder endpoint for move up/move down - prevents any backdoor to updating envs or any permission error but only accepting an array of ids
Updates the /post endpoint to accept the optional projects property (previously, we ignored this)
Updates the bulk update endpoint
Moves logic to fire any web hooks to specific controller methods
Updates EnvironmentModal to consume the PUT and POST requests (previously was using /organization)
Updates ‘environments.tsxto consume the new/environment/order` PUT request
Closes - [Feature] Update environment controller and remove ability to update environments from putOrganization #2494

Testing

Ensure that engineers, experimenters, and admins can still add, update, and remove environments without issue
Ensure that non-engineers, experimenters, and admins can't add, update, remove or change order of environments
Ensure that engineers, experimenters, and admins can move up/move down environments, even if its an environment you don't have permission to edit

… deleted environments from slack

…hanges

mknowlton89 · 2024-05-24T19:16:03Z

packages/back-end/src/routers/environment/environment.controller.ts

@@ -47,9 +101,107 @@ export const putEnvironments = async (
      environments: updatedEnvironments,
    },
  });
+
+  await req.audit({


I looked into adding logic here to fire webhooks if an env's projects were updated, but the final argument to addEnvironmentToOrganizationEnvironments above is false, so we're not replacing any existing envs, we're ignoring them, so no existing env should see any changes.

packages/back-end/src/routers/environment/environment.controller.ts

…hecking for updating order of envs.

github-actions · 2024-05-28T19:19:58Z

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

Preview environment endpoints are available at:

romain-growthbook

Mostly questions and nitpicking stuff. The only strong suggestion is to use loadash to compare projects.

romain-growthbook · 2024-05-28T21:17:21Z

packages/back-end/src/routers/environment/environment.controller.ts

+    const index = existingEnvs.findIndex((env) => env.id === environment);
+
+    if (index < 0) {
+      return res.status(400).json({
+        status: 400,
+        message: `Unable to find environment: ${environment}`,
+      });
+    }
+
+    updatedEnvs.push(existingEnvs[index]);


Any reason for now using the plain find method?

Looks like the same code as putEnvironments. Maybe it could be factored out?

No good reason for using find vs findIndex. I'll update to find as it does make things a bit more concise.

Actually, I think for the putEnvironment it makes sense to keep the findIndex as with that, we want to find the index, so we can later update that specific index. I think find is better for the putEnvironmentOrder and deleteEnvironment, though.

packages/back-end/src/routers/environment/environment.controller.ts

romain-growthbook · 2024-05-28T21:27:16Z

packages/back-end/src/routers/environment/environment.controller.ts

+      const existingProjects = envsArr[existingEnvIndex].projects || [];
+      const newProjects = environment.projects || [];
+
+      if (JSON.stringify(existingProjects) !== JSON.stringify(newProjects)) {


This comparison is bound to be runtime dependent on the order of keys in the object. lodash is used in the code already and has a comparison utility that seems better suited: https://lodash.com/docs/4.17.15#isEqual

packages/back-end/src/routers/environment/environment.controller.ts

romain-growthbook · 2024-05-28T21:31:59Z

packages/back-end/src/routers/environment/environment.router.ts

+        environment: z
+          .object({
+            id: z.string(),
+            description: z.string(),
+            toggleOnList: z.boolean().optional(),
+            defaultState: z.any().optional(),
+            projects: z.array(z.string()).optional(),
+          })
+          .strict(),


Should this be factored out to make it more visible for potential future re-use?

romain-growthbook · 2024-05-28T21:33:06Z

packages/back-end/types/audit.d.ts

+  | "environment.create"
+  | "environment.update"
+  | "environments.update"
+  | "environment.delete"


Do you also need an entry in EntityType?

Oof - yes - great catch!

When doing the bulk updates - either via the putEnvironmentOrder or putEnvironments - there isn't a single env.id for the entity, so I've currently just got it set to the organization. Not sure if that's the right direction...

I have a PR changing those type definitions to take a constant object mapping entities to audit event names. This should fix these kind of gaps.

romain-growthbook · 2024-05-28T21:34:22Z

packages/front-end/pages/environments.tsx

@@ -222,12 +222,10 @@ const EnvironmentsPage: FC = () => {
                                const newEnvs = [...environments];
                                newEnvs.splice(i, 1);
                                newEnvs.splice(i - 1, 0, e);
-                                await apiCall(`/organization`, {
+                                await apiCall(`/environment/order`, {


Do we need a net new endpoint for this? Should we simply allow the plural endpoint to also update the order?

I ultimately broke it out into a separate endpoint as the permission logic for each of these is different.

For re-ordering, the user only needs to have permission to create environments globally, or in at least 1 project. Whereas, if the user is updating the actual environments, they need create/update permission for every endpoint passed in.

If we added this to the plural endpoint, the logic would get really complicated. We'd need to do a deep comparison of the environments being passed in to see if the user was just trying to update the order, or update the actual environments.

Ultimately, it felt like it would lead to bugs, and was easily handled by breaking the logic out into two separate endpoints.

romain-growthbook

Looks good now! Maybe once the integration tests basic stuff drops we could consider adding tests for those endpoints as well.

Remove update environments from putOrganization.

442d654

mknowlton89 self-assigned this May 24, 2024

mknowlton89 added 4 commits May 24, 2024 11:24

Updates env controller and router to introduce/update new routes.

de6a5e8

updates router to use the strict call

77826a6

Adds audit logging to deleteEnvironment and also adds logic to remove…

99b3305

… deleted environments from slack

Adds logging and logic to fire webhook updates if an env's projects c…

1b7ee35

…hanges

mknowlton89 commented May 24, 2024

View reviewed changes

mknowlton89 commented May 28, 2024

View reviewed changes

packages/back-end/src/routers/environment/environment.controller.ts Show resolved Hide resolved

mknowlton89 commented May 28, 2024

View reviewed changes

packages/back-end/src/routers/environment/environment.controller.ts Outdated Show resolved Hide resolved

mknowlton89 added 2 commits May 28, 2024 07:26

Normalizes methods to all have try catch blocks and adds permission c…

da8c682

…hecking for updating order of envs.

Merge branch 'main' into mk/remove-env-update-from-putOrganization

f6bcc3f

mknowlton89 marked this pull request as ready for review May 28, 2024 19:05

romain-growthbook reviewed May 28, 2024

View reviewed changes

Addresses Romain's feedback and suggestions

e1b1bb3

mknowlton89 requested a review from romain-growthbook May 29, 2024 13:58

romain-growthbook approved these changes May 30, 2024

View reviewed changes

mknowlton89 added 3 commits May 30, 2024 13:10

Merges in main and resolves conficts

a0948f1

Uses new event type

d9b58fd

Removes MKTODOs

de71b2a

mknowlton89 merged commit 18c87e2 into main May 30, 2024
3 checks passed

mknowlton89 deleted the mk/remove-env-update-from-putOrganization branch May 30, 2024 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove update environments from putOrganization. #2564

Remove update environments from putOrganization. #2564

mknowlton89 commented May 23, 2024 •

edited

Loading

mknowlton89 May 24, 2024

github-actions bot commented May 28, 2024 •

edited

Loading

romain-growthbook left a comment

romain-growthbook May 28, 2024

romain-growthbook May 28, 2024

mknowlton89 May 29, 2024 •

edited

Loading

mknowlton89 May 29, 2024

romain-growthbook May 28, 2024

romain-growthbook May 28, 2024

romain-growthbook May 28, 2024

mknowlton89 May 29, 2024

mknowlton89 May 29, 2024

romain-growthbook May 30, 2024

romain-growthbook May 28, 2024

mknowlton89 May 29, 2024

romain-growthbook left a comment

Remove update environments from putOrganization. #2564

Remove update environments from putOrganization. #2564

Conversation

mknowlton89 commented May 23, 2024 • edited Loading

Features and Changes

Testing

Choose a reason for hiding this comment

github-actions bot commented May 28, 2024 • edited Loading

romain-growthbook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mknowlton89 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romain-growthbook left a comment

Choose a reason for hiding this comment

mknowlton89 commented May 23, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

mknowlton89 May 29, 2024 •

edited

Loading