API environment endpoints #2529
Conversation
|
Your preview environment pr-2529-bttf has been deployed. |
|
Deploy preview for docs ready! ✅ Preview Built with commit 1a4ff32. |
| }, | ||
| ], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Lots of copy/pasting in this test files makes it a little hard to follow and maintain. For example, if we add a new method to req.context that is needed under-the-hood, we will have to update it 13 times in this file.
There was a problem hiding this comment.
It's actually the opposite, the context is only setting what's needed for the API call to consume. If a new method is added, the tests don't need to worry about it. The only case they'd need to be edited is if one of the API the implementation rely on changes, for instance if a permission method is modified, which is what we want to catch.
Generally speaking, I lean more and more toward making my tests more verbose and duplicated, contrary to actual code. This adds some maintenance burden but seems to make the tests more robust. The problem with factoring out too much is that abstract code is more prone to silent issues that would make all tests seemingly pass because, for instance, one higher order loop, is not operating the way it needs to. Having all tests be rigid and and less smart makes it more obvious when something breaks.
That being said, the code needs some factorization of the setup. I have added a api.setup.ts utility function that handles the app and request context setup and teardown. Now the tests only have to specify what request context they want. Should make it more readable!
| properties: | ||
| description: | ||
| type: string | ||
| description: The description of the new environment |
There was a problem hiding this comment.
nit: Should probably remove the new reference here.
| } | ||
|
|
||
| if (!req.context.permissions.canDeleteEnvironment(environment)) | ||
| throw Error("You do not have permission to delete this environment!"); |
There was a problem hiding this comment.
We have a specific permissions error that we can use that provides better categorization of errors.
E.G.
if (!context.permissions.canDeleteEnvironment(environment)) {
context.permissions.throwPermissionError();
}
romain-growthbook
force-pushed
the
env-api-endpoints
branch
from
8fa9dde
to
26965c8
Compare
|
All PR comments have been addressed now! |
| environments: environments.reduce( | ||
| (ret, env) => [...ret, env.id === id ? updatedEnvironment : env], | ||
| [] | ||
| ), |
There was a problem hiding this comment.
Minor code style, but map makes a lot more sense here than reduce. I tend to avoid reduce whenever possible since it's usually much harder to read and follow the logic compared to alternatives.
romain-growthbook
force-pushed
the
env-api-endpoints
branch
from
4cda76c
to
1a4ff32
Compare
No description provided.