fix: seed OAuth IS_*_ENABLED flags individually instead of all-or-nothing#8740
fix: seed OAuth IS_*_ENABLED flags individually instead of all-or-nothing#8740rucoder wants to merge 1 commit intomakeplane:previewfrom
Conversation
📝 WalkthroughWalkthroughRefactors OAuth flag seeding in the configure_instance management command to independently seed each IS_*_ENABLED key using per-key checks and get_or_create, adds unit tests for seeding behavior, and removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as configure_instance (management cmd)
participant Env as Environment / instance_config_variables
participant DB as InstanceConfiguration (DB)
Cmd->>Env: read per-provider config checks (get_configuration_value)
loop for each provider key
Cmd->>DB: InstanceConfiguration.objects.get_or_create(key=IS_*_ENABLED, defaults={value: "1"/"0", category: AUTHENTICATION})
alt created
DB-->>Cmd: created -> Cmd prints success
else exists
DB-->>Cmd: exists -> Cmd prints warning
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
7343ba0 to
c5f05d7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the configure_instance management command used an all-or-nothing guard (exists() on a group of keys) that was always short-circuiting because IS_GITEA_ENABLED was already seeded by the instance_config_variables loop — leaving IS_GOOGLE_ENABLED, IS_GITHUB_ENABLED, and IS_GITLAB_ENABLED rows never created. It also adds whitespace trimming to the configuration PATCH endpoint.
Changes:
- Replaces the all-or-nothing
exists()guard with individualget_or_createcalls per OAuthIS_*_ENABLEDkey, fixing the root cause described in issue #8739. - Adds
str(...).strip()to thepatchmethod inInstanceConfigurationEndpointso configuration values are trimmed of surrounding whitespace on save. - Adds a new unit test class (
TestInstanceConfigurationWhitespaceTrimming) covering the trimming behavior for plain-text configuration fields.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
apps/api/plane/license/management/commands/configure_instance.py |
Replaces bulk exists() guard with per-key get_or_create to independently seed each IS_*_ENABLED OAuth flag |
apps/api/plane/license/api/views/configuration.py |
Wraps request value in str(...).strip() before saving, trimming accidental whitespace in configuration values |
apps/api/plane/tests/unit/views/test_instance_configuration.py |
New test file with four tests verifying that leading/trailing whitespace is stripped from configuration values in the PATCH endpoint |
apps/api/plane/tests/unit/views/__init__.py |
New empty __init__.py for the new test subdirectory |
c5f05d7 to
b1ebaad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/plane/license/management/commands/configure_instance.py (1)
84-96: This still needs a command-level regression test.The main bug fixed by this block is the partial-state seed case, but the new coverage in this PR only exercises
/api/instances/configurations/. Please add aconfigure_instancetest whereIS_GITEA_ENABLEDalready exists and the other three rows do not, then assert the command creates the missing flags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/license/management/commands/configure_instance.py` around lines 84 - 96, Add a command-level regression test for the configure_instance management command that exercises the partial-seed case: create an InstanceConfiguration row for "IS_GITEA_ENABLED" beforehand and ensure the other keys from oauth_enabled_keys do not exist, then run the configure_instance command and assert it creates the missing InstanceConfiguration rows while leaving the existing "IS_GITEA_ENABLED" intact; use the InstanceConfiguration model to set up and assert state and import/run the configure_instance command (or call Command().handle()) so the test directly verifies the behavior implemented in the loop that reads oauth_enabled_keys and calls InstanceConfiguration.objects.get_or_create.apps/api/plane/tests/unit/views/test_instance_configuration.py (1)
54-132: Please add anullregression case here.These tests only PATCH string values, so they would not catch the
null -> "None"behavior inInstanceConfigurationEndpoint.patch(). One plaintext case and one encrypted case would lock down the branch this PR changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/unit/views/test_instance_configuration.py` around lines 54 - 132, Add two regression tests in test_instance_configuration.py that exercise the null->"None" bug in InstanceConfigurationEndpoint.patch(): one for a non-encrypted config and one for an encrypted config. For each test, create the InstanceConfiguration (use keys like "GITHUB_CLIENT_ID" and an encrypted key variant), send admin_client.patch to "/api/instances/configurations/" with the key mapped to None (JSON null), assert response.status_code == 200, then reload the InstanceConfiguration and assert its value remained null (or unchanged from its original null/blank state) rather than becoming the string "None"; this locks down the branch in InstanceConfigurationEndpoint.patch().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/license/api/views/configuration.py`:
- Around line 48-50: The current logic in configuration handling converts any
incoming value to str and .strip(), which turns JSON null into the literal
"None"; update the logic in the code around the request.data lookup (the block
that computes value for configuration.key in configuration.py) to preserve
non-string values (including None) and only call .strip() when the incoming
value is already a str—i.e., get the raw_value =
request.data.get(configuration.key, configuration.value), then if
isinstance(raw_value, str) set value = raw_value.strip(), otherwise set value =
raw_value so encrypted configs aren't corrupted by turning null into "None".
---
Nitpick comments:
In `@apps/api/plane/license/management/commands/configure_instance.py`:
- Around line 84-96: Add a command-level regression test for the
configure_instance management command that exercises the partial-seed case:
create an InstanceConfiguration row for "IS_GITEA_ENABLED" beforehand and ensure
the other keys from oauth_enabled_keys do not exist, then run the
configure_instance command and assert it creates the missing
InstanceConfiguration rows while leaving the existing "IS_GITEA_ENABLED" intact;
use the InstanceConfiguration model to set up and assert state and import/run
the configure_instance command (or call Command().handle()) so the test directly
verifies the behavior implemented in the loop that reads oauth_enabled_keys and
calls InstanceConfiguration.objects.get_or_create.
In `@apps/api/plane/tests/unit/views/test_instance_configuration.py`:
- Around line 54-132: Add two regression tests in test_instance_configuration.py
that exercise the null->"None" bug in InstanceConfigurationEndpoint.patch(): one
for a non-encrypted config and one for an encrypted config. For each test,
create the InstanceConfiguration (use keys like "GITHUB_CLIENT_ID" and an
encrypted key variant), send admin_client.patch to
"/api/instances/configurations/" with the key mapped to None (JSON null), assert
response.status_code == 200, then reload the InstanceConfiguration and assert
its value remained null (or unchanged from its original null/blank state) rather
than becoming the string "None"; this locks down the branch in
InstanceConfigurationEndpoint.patch().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5e4437b-eb4c-4f30-9bb2-e3c19be9d25d
📒 Files selected for processing (4)
apps/api/plane/license/api/views/configuration.pyapps/api/plane/license/management/commands/configure_instance.pyapps/api/plane/tests/unit/views/__init__.pyapps/api/plane/tests/unit/views/test_instance_configuration.py
| value = str( | ||
| request.data.get(configuration.key, configuration.value) | ||
| ).strip() |
There was a problem hiding this comment.
Only trim actual strings here.
str(...).strip() turns a JSON null into the literal "None". For encrypted configs that value then gets encrypted and saved, so a PATCH meant to clear a setting silently corrupts it instead. Preserve None and only trim when the incoming value is already a string.
💡 Suggested fix
- value = str(
- request.data.get(configuration.key, configuration.value)
- ).strip()
+ value = request.data.get(configuration.key, configuration.value)
+ if isinstance(value, str):
+ value = value.strip()
+ elif value is not None:
+ value = str(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/license/api/views/configuration.py` around lines 48 - 50, The
current logic in configuration handling converts any incoming value to str and
.strip(), which turns JSON null into the literal "None"; update the logic in the
code around the request.data lookup (the block that computes value for
configuration.key in configuration.py) to preserve non-string values (including
None) and only call .strip() when the incoming value is already a str—i.e., get
the raw_value = request.data.get(configuration.key, configuration.value), then
if isinstance(raw_value, str) set value = raw_value.strip(), otherwise set value
= raw_value so encrypted configs aren't corrupted by turning null into "None".
There was a problem hiding this comment.
This is a stale comment — after rebasing, this PR no longer touches configuration.py. The whitespace trimming fix is in PR #8738.
There was a problem hiding this comment.
🧩 Analysis chain
@rucoder, thanks for the clarification! You're right — the concern is no longer applicable to this PR after the rebase. I can see the fix was handled in #8738. I'll dismiss this comment.
🏁 Script executed:
#!/bin/bash
# Resolve the stale review comment
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOHBYjkM5zyKh1"}) {
thread {
isResolved
}
}
}'
Length of output: 301
Let me look up the correct thread ID for this comment.
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
{
repository(owner: "makeplane", name: "plane") {
pullRequest(number: 8740) {
reviewThreads(first: 20) {
nodes {
id
isResolved
path
comments(first: 1) {
nodes {
body
author { login }
}
}
}
8000
}
}
}
}'
Length of output: 6223
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOIdx9Rs5zEesV"}) {
thread {
isResolved
}
}
}'
Length of output: 265
@rucoder, thanks for the heads-up! You're correct — this comment is stale and no longer applies to this PR after the rebase. The whitespace trimming concern was addressed in #8738.
Unfortunately, I'm unable to programmatically resolve this thread due to GitHub integration permission restrictions. You can manually mark it as resolved using the Resolve conversation button on the PR.
🧠 Learnings used
Learnt from: dheeru0198
Repo: makeplane/plane PR: 8339
File: apps/api/plane/db/models/api.py:35-35
Timestamp: 2025-12-23T14:18:32.899Z
Learning: Django REST Framework rate limit strings are flexible: only the first character of the time unit matters. Acceptable formats include: "60/s", "60/sec", "60/second" (all equivalent), "60/m", "60/min", "60/minute" (all equivalent), "60/h", "60/hr", "60/hour" (all equivalent), and "60/d", "60/day" (all equivalent). Abbreviations like "min" are valid and do not need to be changed to "minute". Apply this guidance to any Python files in the project that configure DRF throttling rules.
Learnt from: anton-v-a
Repo: makeplane/plane PR: 8588
File: apps/api/plane/app/views/view/base.py:246-247
Timestamp: 2026-01-31T22:16:00.468Z
Learning: In this repo, copy.deepcopy is the established approach for cloning Django QuerySets before applying annotations. When reviewing Python files that manipulate QuerySets for annotations, prefer copy.deepcopy and avoid suggesting .all() in isolation. Ensure consistency with existing usage in makeplane/plane (e.g., issue/base.py, workspace/user.py, module/issue.py, cycle/issue.py, issue/archive.py). Apply this pattern in similar code paths (e.g., apps/api/plane/app/views/view/base.py) to maintain consistency across the codebase.
| "IS_GITEA_ENABLED": lambda: all( | ||
| get_configuration_value( | ||
| [ | ||
| {"key": "GITEA_HOST", "default": os.environ.get("GITEA_HOST", "")}, | ||
| {"key": "GITEA_CLIENT_ID", "default": os.environ.get("GITEA_CLIENT_ID", "")}, | ||
| {"key": "GITEA_CLIENT_SECRET", "default": os.environ.get("GITEA_CLIENT_SECRET", "")}, | ||
| ] | ||
| ) | ||
| ), |
There was a problem hiding this comment.
The IS_GITEA_ENABLED entry in oauth_enabled_keys is redundant and its lambda will never actually be used to create a row. Because IS_GITEA_ENABLED is already included in gitea_config_variables inside instance_config_variables (see core.py line 116), it is always seeded by the first get_or_create loop (lines 29–41). By the time the second loop reaches IS_GITEA_ENABLED, the row already exists, so get_or_create will never call the lambda or create anything — it will always take the else branch and print "configuration already exists".
The IS_GITEA_ENABLED entry should be removed from oauth_enabled_keys to avoid confusion. If the seeding logic for IS_GITEA_ENABLED should instead mirror the credential-check approach used for the other three providers (checking GITEA_HOST, GITEA_CLIENT_ID, and GITEA_CLIENT_SECRET), then its entry in gitea_config_variables in core.py should be removed, and the oauth_enabled_keys entry kept. The current state creates a misleading impression that all four keys are handled consistently when IS_GITEA_ENABLED actually follows a completely different seeding path.
There was a problem hiding this comment.
Good point. Removed IS_GITEA_ENABLED from gitea_config_variables in core.py so all four flags are now handled consistently in configure_instance.py with category AUTHENTICATION. Also added a test (test_all_enabled_flags_use_authentication_category) that verifies this consistency.
| # Seed IS_*_ENABLED flags individually so that each missing key | ||
| # is created independently. The previous all-or-nothing check | ||
| # skipped every key when even one already existed (e.g. | ||
| # IS_GITEA_ENABLED seeded via instance_config_variables). | ||
| oauth_enabled_keys = { |
There was a problem hiding this comment.
The auto-generated CodeRabbit summary in the PR description mentions two features not present in the diff:
- "Configuration values submitted via the API are now automatically trimmed of leading and trailing whitespace"
- "Added unit tests verifying configuration value whitespace trimming"
Neither of these changes appears in the provided diff. If these were intended to be part of this PR, they are missing from the changeset. If they were generated in error (i.e., the auto-summary hallucinated them), the PR description should be updated to remove those bullet points to avoid confusion for reviewers.
There was a problem hiding this comment.
That was from the auto-generated CodeRabbit summary which hallucinated changes from the sibling PR #8738. Updated the PR description to only describe the seeding fix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/management/test_configure_instance.py (1)
79-100: Add a positive-path test forvalue == "1".The suite proves row creation and the
"0"fallback, but it never exercises the branch where credentials are present. A typo in one provider’s env-key mapping would still pass these tests.Possible parametrized test
+ `@pytest.mark.django_db` + `@pytest.mark.parametrize`( + ("env", "expected_key"), + [ + ( + {"GOOGLE_CLIENT_ID": "id", "GOOGLE_CLIENT_SECRET": "secret"}, + "IS_GOOGLE_ENABLED", + ), + ( + {"GITHUB_CLIENT_ID": "id", "GITHUB_CLIENT_SECRET": "secret"}, + "IS_GITHUB_ENABLED", + ), + ( + { + "GITLAB_HOST": "https://gitlab.example.com", + "GITLAB_CLIENT_ID": "id", + "GITLAB_CLIENT_SECRET": "secret", + }, + "IS_GITLAB_ENABLED", + ), + ( + { + "GITEA_HOST": "https://gitea.example.com", + "GITEA_CLIENT_ID": "id", + "GITEA_CLIENT_SECRET": "secret", + }, + "IS_GITEA_ENABLED", + ), + ], + ) + def test_enabled_flags_are_one_with_credentials(self, env, expected_key): + out = StringIO() + with patch.dict("os.environ", {"SECRET_KEY": "test-secret", **env}): + call_command("configure_instance", stdout=out) + + assert InstanceConfiguration.objects.get(key=expected_key).value == "1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/unit/management/test_configure_instance.py` around lines 79 - 100, Add a positive-path unit test that mirrors test_enabled_flags_default_to_zero_without_credentials but supplies valid non-empty OAuth env vars so the management command configure_instance creates/updates InstanceConfiguration rows with value == "1"; specifically, create a new test (e.g., test_enabled_flags_set_to_one_with_credentials) that patches os.environ with non-empty values for GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET, GOOGLE_CLIENT_ID/GOOGLE_CLIENT_SECRET, GITLAB_CLIENT_ID/GITLAB_CLIENT_SECRET and GITEA_CLIENT_ID/GITEA_CLIENT_SECRET, calls call_command("configure_instance", stdout=...), and then iterates the same keys ["IS_GOOGLE_ENABLED","IS_GITHUB_ENABLED","IS_GITLAB_ENABLED","IS_GITEA_ENABLED"] asserting InstanceConfiguration.objects.get(key=key).value == "1" to ensure the enabled-flag branch is exercised and catches any env-key mapping typos.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/plane/tests/unit/management/test_configure_instance.py`:
- Around line 79-100: Add a positive-path unit test that mirrors
test_enabled_flags_default_to_zero_without_credentials but supplies valid
non-empty OAuth env vars so the management command configure_instance
creates/updates InstanceConfiguration rows with value == "1"; specifically,
create a new test (e.g., test_enabled_flags_set_to_one_with_credentials) that
patches os.environ with non-empty values for
GITHUB_CLIENT_ID/GITHUB_CLIENT_SECRET, GOOGLE_CLIENT_ID/GOOGLE_CLIENT_SECRET,
GITLAB_CLIENT_ID/GITLAB_CLIENT_SECRET and GITEA_CLIENT_ID/GITEA_CLIENT_SECRET,
calls call_command("configure_instance", stdout=...), and then iterates the same
keys
["IS_GOOGLE_ENABLED","IS_GITHUB_ENABLED","IS_GITLAB_ENABLED","IS_GITEA_ENABLED"]
asserting InstanceConfiguration.objects.get(key=key).value == "1" to ensure the
enabled-flag branch is exercised and catches any env-key mapping typos.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18cc4612-dbbd-43c2-9786-aea8aa9b4321
📒 Files selected for processing (3)
apps/api/plane/license/management/commands/configure_instance.pyapps/api/plane/tests/unit/management/__init__.pyapps/api/plane/tests/unit/management/test_configure_instance.py
…hing The configure_instance management command checked whether *any* of the four IS_*_ENABLED keys (Google, GitHub, GitLab, Gitea) existed before deciding whether to create them. Because IS_GITEA_ENABLED was already seeded by instance_config_variables in the first loop, the exists() check always returned True and the remaining three keys were silently skipped. This meant IS_GITHUB_ENABLED, IS_GOOGLE_ENABLED, and IS_GITLAB_ENABLED were never created in the database. Toggling these providers on in God Mode sent a PATCH to update a non-existent row, which returned 200 OK but did nothing — the toggle appeared to succeed but reverted on page reload. Replace the all-or-nothing check with per-key get_or_create so each flag is seeded independently of the others. Move IS_GITEA_ENABLED out of instance_config_variables (core.py) so that all four flags are handled consistently in configure_instance with category AUTHENTICATION. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
b1ebaad to
9e504e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/management/test_configure_instance.py (1)
14-34: Good test coverage for the bug fix.The tests comprehensively cover the key scenarios. One minor robustness improvement: consider using
clear=Truewithpatch.dictto ensure complete isolation from the CI/test runner environment:with patch.dict("os.environ", {"SECRET_KEY": "test-secret"}, clear=True):This prevents any OAuth env vars that might be set in the test environment from affecting the test behavior. Currently it works because these tests only check existence, not values, but
clear=Truewould make the tests more deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/unit/management/test_configure_instance.py` around lines 14 - 34, Update the test TestConfigureInstanceOAuthSeeding.test_creates_all_enabled_flags_when_none_exist to use patch.dict with clear=True so the environment is fully isolated; specifically, change the context manager call that currently uses patch.dict("os.environ", {"SECRET_KEY": "test-secret"}) to include clear=True to prevent stray OAuth env vars from affecting configure_instance behavior during the call_command("configure_instance", stdout=out) invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/plane/tests/unit/management/test_configure_instance.py`:
- Around line 14-34: Update the test
TestConfigureInstanceOAuthSeeding.test_creates_all_enabled_flags_when_none_exist
to use patch.dict with clear=True so the environment is fully isolated;
specifically, change the context manager call that currently uses
patch.dict("os.environ", {"SECRET_KEY": "test-secret"}) to include clear=True to
prevent stray OAuth env vars from affecting configure_instance behavior during
the call_command("configure_instance", stdout=out) invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bb8f54c-7dcb-430b-992f-47f47c6f90b4
📒 Files selected for processing (4)
apps/api/plane/license/management/commands/configure_instance.pyapps/api/plane/tests/unit/management/__init__.pyapps/api/plane/tests/unit/management/test_configure_instance.pyapps/api/plane/utils/instance_config_variables/core.py
💤 Files with no reviewable changes (1)
- apps/api/plane/utils/instance_config_variables/core.py
| config = InstanceConfiguration.objects.get(key=key) | ||
| assert config.value == "0", f"{key} should be '0' without credentials" | ||
|
|
||
| @pytest.mark.django_db |
There was a problem hiding this comment.
The test suite covers the negative path (no credentials → IS_*_ENABLED="0") but is missing a test for the positive path: when OAuth credentials are provided, the corresponding IS_*_ENABLED flag should be set to "1". Since SKIP_ENV_VAR defaults to True (DB mode), such a test would need to pre-create the credential rows (e.g., GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET) with non-empty values in the DB, then call configure_instance and assert that IS_GITHUB_ENABLED has value "1". Without this test, the credential-presence check logic in the lambda (line 86) is not exercised end-to-end.
| @pytest.mark.django_db | |
| @pytest.mark.django_db | |
| def test_github_flag_set_to_one_with_db_credentials(self): | |
| """With GitHub OAuth credentials in DB, IS_GITHUB_ENABLED should be '1'.""" | |
| # Pre-create non-empty GitHub credential rows in the database (DB mode). | |
| InstanceConfiguration.objects.create( | |
| key="GITHUB_CLIENT_ID", | |
| value="github-client-id", | |
| category="AUTHENTICATION", | |
| is_encrypted=False, | |
| ) | |
| InstanceConfiguration.objects.create( | |
| key="GITHUB_CLIENT_SECRET", | |
| value="github-client-secret", | |
| category="AUTHENTICATION", | |
| is_encrypted=False, | |
| ) | |
| out = StringIO() | |
| # SKIP_ENV_VAR defaults to True, so the command should read credentials from DB. | |
| with patch.dict("os.environ", {"SECRET_KEY": "test-secret"}): | |
| call_command("configure_instance", stdout=out) | |
| config = InstanceConfiguration.objects.get(key="IS_GITHUB_ENABLED") | |
| assert config.value == "1", "IS_GITHUB_ENABLED should be '1' when DB credentials are present" | |
| @pytest.mark.django_db |
Summary
Fixes #8739
The
configure_instancemanagement command used an all-or-nothing check when seeding OAuthIS_*_ENABLEDflags (IS_GOOGLE_ENABLED,IS_GITHUB_ENABLED,IS_GITLAB_ENABLED,IS_GITEA_ENABLED). BecauseIS_GITEA_ENABLEDwas already seeded bygitea_config_variablesininstance_config_variables(firstget_or_createloop), theexists()check always returnedTrueand the remaining three keys were silently skipped.This caused the God Mode toggle for GitHub/Google/GitLab to appear to succeed (the PATCH returned 200) but actually do nothing — the
IS_*_ENABLEDrow didn't exist in the database, sobulk_updateupdated zero rows.Changes
configure_instance.py: Replace the all-or-nothingif not ...filter(key__in=keys).exists()block with individualget_or_createcalls per key, so each flag is seeded independently.core.py: RemoveIS_GITEA_ENABLEDfromgitea_config_variablesso all fourIS_*_ENABLEDflags are handled consistently inconfigure_instance.pywith categoryAUTHENTICATION(previouslyIS_GITEA_ENABLEDwas the only one seeded separately with categoryGITEA, which is what caused the bug).Tests: Added 5 unit tests covering fresh creation, partial state recovery, idempotency, default values, and category consistency.
Known issue
On existing installs,
IS_GITEA_ENABLEDwas previously seeded withcategory="GITEA"(byinstance_config_variables). Sinceget_or_createonly appliesdefaultswhen creating new rows, existing instances will keep the old category. This is not functionally broken — the frontend and backend look up configuration by key, not by category — but it's a cosmetic inconsistency. A follow-up data migration could normalize it toAUTHENTICATIONfor consistency, but is not required for correctness.Testing
Verified manually on a self-hosted v1.2.3 instance where
IS_GITEA_ENABLEDexisted but the other threeIS_*_ENABLEDrows were missing. After applying this fix and restarting the API container, all four rows were created correctly.Summary by CodeRabbit
Bug Fixes
Tests
Chores