Skip to content

BDMS-501: Implement read-only admin view for NMAMinorTraceChemistry#434

Merged
chasetmartin merged 12 commits into
stagingfrom
kas-bdms-501-admin-view-NMAMinorTraceChemistry
Jan 29, 2026
Merged

BDMS-501: Implement read-only admin view for NMAMinorTraceChemistry#434
chasetmartin merged 12 commits into
stagingfrom
kas-bdms-501-admin-view-NMAMinorTraceChemistry

Conversation

@ksmuczynski

Copy link
Copy Markdown
Contributor

Why

This PR addresses the following problem / context:

  • MinorTraceChemistry model was missing legacy fields. All legacy fields need to be brought over in the 1:1 migration.
  • Align the Minor Trace Chemistry admin view with recent model changes (renamed FK and added fields).
  • Keep admin configuration and tests consistent with legacy NMA naming conventions.

How

Implementation summary - the following was changed / added / removed:

  • Updated MinorTraceChemistryAdmin list/form/sort/search fields and labels to include sample_pt_id, sample_point_id, object_id, and wclab_id while keeping the chemistry_sample_info relationship.
  • Adjusted admin view tests to reflect the updated field set and view label/name.
  • Fixed the NMA_MinorTraceChemistry unique constraint to reference the mapped legacy column name (Analyte).

Notes

Any special considerations, workarounds, or follow-up work to note?

  • Tests run: pytest tests/test_admin_minor_trace_chemistry.py (passes; dependency deprecation warnings from starlette_admin and SQLAlchemy UTC usage).

…rTraceChemistry

This aligns with the 1:1 migration, preserving all legacy field names.
…e model, and aligned the admin tests with the new configuration.

Details:

  - Added sample_pt_id, sample_point_id, object_id, and wclab_id to list/sort/search/form configs in admin/views/minor_trace_chemistry.py.
  - Updated field labels to match legacy column naming.
  - Adjusted expectations in tests/test_admin_minor_trace_chemistry.py to match the new fields/labels.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the NMAMinorTraceChemistry model and admin view to align with legacy database field mappings and naming conventions. The changes ensure proper 1:1 migration compatibility by adding missing legacy fields and correcting foreign key references.

Changes:

  • Renamed foreign key column from chemistry_sample_info_id to sample_pt_id and updated unique constraint to reference correct legacy column name Analyte
  • Added missing legacy fields (sample_point_id, object_id, wclab_id) with proper column mappings
  • Updated admin view configuration to include new fields in list, form, sortable, and searchable field sets
  • Updated tests and view labels to reflect "NMA Minor Trace Chemistry" naming convention

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
db/nma_legacy.py Updated model to use correct FK name, added missing legacy fields with proper column mappings, and fixed unique constraint
admin/views/minor_trace_chemistry.py Added new fields to admin configuration including list, sortable, searchable fields and field labels
tests/test_admin_minor_trace_chemistry.py Updated test assertions to expect "NMA Minor Trace Chemistry" label and verify new fields are present

Comment thread db/nma_legacy.py
Comment thread admin/views/minor_trace_chemistry.py
@ksmuczynski ksmuczynski marked this pull request as draft January 27, 2026 19:22
  - map SamplePtID in NMA_MinorTraceChemistry and fix unique constraint
  - update minor trace admin integration test for NMA label/sample_pt_id
  - add alembic migration to align legacy column names and merge heads
- rename expected FK field from chemistry_sample_info_id to sample_pt_id
- include new legacy columns in expected minor trace model fields
- replace chemistry_sample_info_id with sample_pt_id in the admin feature field list
- why: model/admin config now exposes the legacy SamplePtID mapping, so the feature spec must align with the current schema naming
@ksmuczynski ksmuczynski marked this pull request as ready for review January 27, 2026 20:19
Copilot AI review requested due to automatic review settings January 27, 2026 20:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comment thread alembic/versions/4f6b7c8d9e0f_merge_minor_trace_and_field_parameters.py Outdated
- Update 3a9c1f5b7d2e to point at c1d2e3f4a5b6
- Remove the obsolete merge revision 4f6b7c8d9e0f
- Reason: manual rebase to realign the migration chain after history changes and avoid a redundant merge node

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment thread db/nma_legacy.py

@chasetmartin chasetmartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I just noticed your updated names weren't being used in the transfer script, so I just pushed a quick update to change those names and the transfer was working on my end.

@chasetmartin chasetmartin merged commit 444ed13 into staging Jan 29, 2026
6 checks passed
@TylerAdamMartinez TylerAdamMartinez deleted the kas-bdms-501-admin-view-NMAMinorTraceChemistry branch February 5, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants