Skip to content

Integrate sync_schema_config_fields fixes into main#8067

Open
acwhite211 wants to merge 2 commits into
mainfrom
issue-8065
Open

Integrate sync_schema_config_fields fixes into main#8067
acwhite211 wants to merge 2 commits into
mainfrom
issue-8065

Conversation

@acwhite211
Copy link
Copy Markdown
Member

@acwhite211 acwhite211 commented May 8, 2026

Fixes #8065

Integrate sync_schema_config_fields fixes into main.

This ports the release branch schema config fix into main while preserving the newer main-branch schema config locking and de-duplication logic.

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone
  • Add pr to documentation list

Testing instructions

  • Run ve/bin/python manage.py sync_schema_config_fields --discipline-id <discipline_id>.
  • Confirm the “Missing fields” output does not include internal ID fields like accessionId, localityId, collectionObjectId, or other {table}Id fields.
  • Confirm legitimate missing non-ID fields still appear in the output.
  • Run ve/bin/python manage.py sync_schema_config_fields --discipline-id <discipline_id> --apply.
  • Confirm created schema config records are limited to expected non-ID fields.
  • Re-run the command and confirm previously created fields are no longer reported as missing.

Summary by CodeRabbit

  • Refactor
    • Improved internal schema configuration field handling to use a centralized helper for field iteration.
    • Enhanced data model field collection logic for better consistency in field filtering across migration utilities.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b31f0377-6257-40ab-bbc2-1195e85241d9

📥 Commits

Reviewing files that changed from the base of the PR and between b3a6255 and 6bc3377.

📒 Files selected for processing (2)
  • specifyweb/specify/migration_utils/update_schema_config.py
  • specifyweb/specify/models_utils/load_datamodel.py

📝 Walkthrough

Walkthrough

A new _all_fields() generator method is added to the Table class to centralize field iteration with optional exclusion of ID fields, virtual fields, and relationships. The existing all_fields property is refactored to delegate to this helper. Schema config migration functions are updated to use the new helper for consistent ID field filtering.

Changes

Field Iteration Helper and Migration Updates

Layer / File(s) Summary
_all_fields generator and all_fields refactor
specifyweb/specify/models_utils/load_datamodel.py
New _all_fields() generator centralizes field/relationship collection with parameters to optionally exclude fields, relationships, ID field, and virtual fields. all_fields property now delegates to this helper and includes updated docstring.
Schema config migration updates using new helper
specifyweb/specify/migration_utils/update_schema_config.py
update_table_schema_config_with_defaults and find_missing_schema_config_fields are updated to call _all_fields(exclude_id_field=True) instead of manually filtering all_fields. Both functions now rely on the helper for consistent ID field exclusion.
🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Automatic Tests ⚠️ Warning PR modifies critical functions without adding automated tests. Manual testing provided but no unit/integration tests for _all_fields method or modified schema_config functions. Add unit tests for Table._all_fields() with exclude_id_field parameter, and integration tests for find_missing_schema_config_fields to verify ID fields are properly excluded.
Testing Instructions ⚠️ Warning Instructions lack clarity (missing discipline_id guidance, environment setup) and don't cover all affected components (don't test _all_fields parameters or all_fields backward compatibility). Add setup instructions with discipline_id discovery; clarify "legitimate fields"; add tests for _all_fields parameters, backward compatibility, and virtual fields handling; provide output examples.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: integrating fixes into the main branch related to schema config fields.
Linked Issues check ✅ Passed The code changes directly address issue #8065 by refactoring field collection logic to exclude ID fields via a new _all_fields helper method, enabling sync_schema_config_fields to properly avoid creating records for {table}Id fields.
Out of Scope Changes check ✅ Passed All changes are scoped to the stated objectives: refactoring field iteration logic in load_datamodel.py and updating schema-config population functions to use the new exclude_id_field parameter.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-8065

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@acwhite211 acwhite211 added this to the 7.12.1 milestone May 18, 2026
@acwhite211 acwhite211 marked this pull request as ready for review May 18, 2026 21:55
@acwhite211 acwhite211 requested review from a team and removed request for a team May 18, 2026 21:55
@grantfitzsimmons
Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📋Back Log

Development

Successfully merging this pull request may close these issues.

Prevent sync_schema_config_fields from creating schema records for {table}Id fields

2 participants