Enhance proposals endpoint with optional username, cycle, and SAF status filters for MX Webapp

[Aditi Chikkali (AditiChikkali)]

Summary

This PR enhances the v1/proposals endpoint to add optional filtering by username, cycle, and SAF status, improving accuracy and enabling user‑scoped queries without introducing new endpoints to support MX workflow needs for user‑specific proposal retrieval.

Key Changes

• Updated proposals fetch service to accept an optional username filter.
• Updated proposals API route to pass optional filters and apply post‑filters when provided:
  • username
  • current_cycle_only
  • saf_status
• Added unit tests for username filter behavior

Behavior & Compatibility

• Preserved existing endpoint path and response schema.
• Applied current‑cycle filtering only when explicitly requested.
• Added optional SAF status filtering when saf_status is provided from Approved, Draft, Expired, Ressafrev.
• Username filtering now happens before pagination for more accurate results.
• All changes are backward‑compatible and do not affect default behavior when filters are not supplied.

[Copilot]

Pull request overview

This PR enhances the v1/proposals/ admin endpoint to support optional filtering intended for MX Webapp workflows (username-scoped queries, current-cycle-only selection, and SAF status filtering) while keeping the endpoint path and response schema unchanged.

Changes:

• Added an optional username filter to the proposals fetch service (query-level filtering before pagination).
• Extended the v1/proposals/ API route with username, current_cycle_only, and saf_status query parameters.
• Added route-level post-filtering to constrain returned SAF entries (and optionally proposals) based on provided filters.

Reviewed changes

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

File	Description
src/nsls2api/services/proposal_service.py	Adds optional username-based DB filtering to fetch_proposals.
src/nsls2api/api/v1/proposal_api.py	Wires new query params into the proposals endpoint and applies post-filter logic for cycles/SAFs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Padraic Shafer (padraic-shafer)]

I've included recommendations for tightening this up.

[Aditi Chikkali (AditiChikkali)]

Hi Padraic Shafer (@padraic-shafer)

1. I want to confirm my understanding of your comment about moving the current cycle logic out of the API block, if you could check the recent changes please. My intent was to keep current_cycle_only as a separate call, and the current cycle is resolved server-side rather than by the client. Is that the behavior you wanted, or would you prefer a different solution?

2. For the earlier review comment about username and saf_status defaults, I followed the existing style first, then checked Python behavior. For list parameters, [] can be shared across calls, so I changed them to None and normalize inside the function. For string parameters, "" is not a mutable-default bug, but None better represents “no filter provided.” Does that approach look acceptable to you?

[Padraic Shafer (padraic-shafer)]

Aditi Chikkali (@AditiChikkali) You are absolutely correct that the lists should default to None for the reason you stated. Thank you for catching that.

For strings, it comes down to whether "" or None are meaning values in the context of use -- that is, does either of them mean something other than "no value was provided". In this case all three are equivalent. My preference is to avoid None wherever possible, to avoid special handling and NoneType errors. However, we could as you suggested, stick to the existing None convention in this project -- and save refactoring out None for a future PR.

[Padraic Shafer (padraic-shafer)]

I think current cycle should get resolved client-side.

If a "current cycle" convenience is truly necessary, I think it might fit better into a /v1/facility/{facility}/cycles/current/proposals endpoint, similar to:

• /v1/facility/{facility}/cycle/{cycle}/proposals and
• /v1/facility/{facility}/cycles/current

[Aditi Chikkali (AditiChikkali)]

I think current cycle should get resolved client-side.

If a "current cycle" convenience is truly necessary, I think it might fit better into a /v1/facility/{facility}/cycles/current/proposals endpoint, similar to:

• /v1/facility/{facility}/cycle/{cycle}/proposals and
• /v1/facility/{facility}/cycles/current

You are right the current-cycle is convenience parameter is not strictly necessary.

My thinking was mainly, when we pass a cycle filter, the returned proposal can still show other historical cycles in its cycle list, and for the app form we wanted to clearly restrict and display it as the current cycle context. Since the server already knows the current cycle, I thought we could use that to simplify client handling and keep the form automatically tied to the active cycle.

That said, I agree with your direction and will remove the current_cycle_only parameter, and we could jut make use of existing explicit cycle input.

[Padraic Shafer (padraic-shafer)]

I've requested some changes on the function signatures.

Also, I'm confused by what you are trying to achieve with the SAF handling. The custom logic seems unnecessarily complex to me, so I might be misunderstanding the goal of what you want to happen.