Skip to content

Conversation

@stuartcampbell
Copy link
Collaborator

@stuartcampbell stuartcampbell commented Jul 3, 2025

These changes are to make use of the ORCiD field that will soon be added into the PASS Person and Experimenter models that are used in their API.

These changes should just add a field orcid into the user entry within the proposal document which will be null if the PASS API does not contain this information - or hopefully will contain the actual value if it is there 😄.

I have added a column to display the ORCiD in the proposal "diagnostics" page - i.e. /diagnostics/proposal/{proposal_id}

I have also fixed some code warnings that ruff was complaining about.

@stuartcampbell stuartcampbell requested a review from Copilot July 3, 2025 20:58
@stuartcampbell stuartcampbell marked this pull request as ready for review July 3, 2025 20:59
Copy link
Contributor

Copilot AI left a comment

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 introduces support for ORCiD in the PASS integration by extending data models and templates, and updates service logic and logging refinements. It also includes minor refactors to address lint warnings.

  • Propagate ORCID_ID from PASS models into User.orcid and display in diagnostics.
  • Update template and service logic for new orcid field; convert sets to lists for consistent JSON serialization.
  • Refine logging to use facility_name and add a null-check in update_proposals_with_cycle.

Reviewed Changes

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

File Description
proposal_diagnostics.html Added an ORCiD column and cell in diagnostics table
sync_service.py Mapped ORCID_IDUser.orcid; refined logging, list conversions, and null-check logic
models/proposals.py Added orcid: Optional[str] to User model
models/pass_models.py Added ORCID_ID to PassPerson and PassExperimenter models
Comments suppressed due to low confidence (5)

src/nsls2api/services/sync_service.py:280

  • No existing unit or integration tests cover the mapping of ORCID_ID into User.orcid. Consider adding tests to verify that the ORCiD value is correctly synchronized and represented in the proposal model.
            orcid=user.ORCID_ID,

src/nsls2api/models/proposals.py:24

  • Add a brief docstring or inline comment explaining the intended format and usage of the orcid field (e.g., null vs. valid ORCiD identifiers).
    orcid: Optional[str] = None

src/nsls2api/templates/diagnostics/proposal_diagnostics.html:52

  • [nitpick] The header displays mixed-case ORCiD. For consistency with the standard acronym, consider using ORCID in all uppercase.
                        <th>ORCiD</th>

src/nsls2api/services/sync_service.py:152

  • Verify that facility_name is defined in this scope. If it's undefined, either revert to facility.name or introduce a local facility_name = facility.name assignment.
        f"Cycle information (for {facility_name}) synchronized in {time_taken.total_seconds():,.2f} seconds"

src/nsls2api/services/sync_service.py:359

  • The indentation of this await proposal.save() appears misaligned with its if block. Align it under the if proposal is not None: to avoid syntax errors or unintended logic scope.
                await proposal.save()  # type: ignore[union-attr]

@stuartcampbell stuartcampbell added this to the 0.7 milestone Jul 14, 2025
@stuartcampbell stuartcampbell merged commit ff2318a into NSLS2:main Jul 14, 2025
4 checks passed
@stuartcampbell stuartcampbell deleted the update-pass-orcid-changes branch July 14, 2025 13:57
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