Make app, group, and tag descriptions requirable #350
Conversation
… Marshmallow default loading from nulling out descriptions.
barborico
changed the title
eguerrant
left a comment
eguerrant
left a comment
There was a problem hiding this comment.
If possible, please add tests for these changes
| APP_NAME = os.getenv("APP_NAME", "Access") | ||
|
|
||
| # Require descriptions for apps, groups, and tags | ||
| REQUIRE_DESCRIPTIONS = os.getenv("REQUIRE_DESCRIPTIONS", "False") == "True" |
There was a problem hiding this comment.
Feel free to leave as is but since the bool is being ingested as a string, would we want to be more flexible and allow things like 'true', 'True', 'TRUE' etc?
There was a problem hiding this comment.
Yes, this and other boolean options in this file could be updated to
| REQUIRE_DESCRIPTIONS = os.getenv("REQUIRE_DESCRIPTIONS", "False") == "True" | |
| REQUIRE_DESCRIPTIONS = os.getenv("REQUIRE_DESCRIPTIONS", "false").lower() == "true" |
|
|
||
| old_app_name = app.name | ||
| app = schema.load(request.json, instance=app) | ||
| app = schema.load(request.json, instance=app, partial=True) |
There was a problem hiding this comment.
Why was this and other examples in this PR updated to partial=True
There was a problem hiding this comment.
To more consistently support Marshmallow's default loading without clearing properties omitted in a PUT request. When I added load_default="" to the App description schema for consistency, it broke a unit test that was using partial updates which emptied the description. Hence, c206b4c.
Overview of Changes
Descriptions are important for usability and auditability--let's let Access deployers make them required! This PR adds support for an environment variable
REQUIRE_DESCRIPTIONSwhich, if set toTrue, will make descriptions required when creating/editing apps, groups, and tags.Front end environment loading
I made another change (the second commit) to how Vite loads environment so that it will pull variables set in
.envfiles, but will still use the process env as the final source of truth. This was necessary to be able to test the frontend conditional form field requirements locally (or other config-based behavior like the web app name).Before: Only read from shell/system environment
Now: Read from both .env files AND shell environment, with shell taking precedence
Partial updates for apps, groups, and tags
To more consistently support Marshmallow's default loading without clearing properties omitted in a PUT request, I've updated the schema loading to apply partial updates.
Testing
With
REQUIRE_DESCRIPTIONS=True:With
REQUIRE_DESCRIPTIONS=Falseor omitting the variable altogether, the original behavior is preserved.