Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Summary endpoint revision and updates #584

Draft
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

rathod-b
Copy link
Collaborator

@rathod-b rathod-b commented May 8, 2024

Please check if the PR fulfills these requirements

  • CHANGELOG.md is updated
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

(Bug fix, feature, docs update, ...)
Features

What is the current behavior?

(You can also link to an open issue here)

  • Currently, multiple calls have to be made to view summary of runs linked to portfolios and standalone runs. These changes introduce new endpoints to simplify this process and provide speed boost.
  • Currently, not all technology sizes are returned in the summary endpoint
  • Currently, a run cannot be marked separate from a portfolio
  • Currently, Django query is very simple and relies on external code for filtering

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
No changes to the users, they should notice a speedup on Past Evaluations page

Other information:

@rathod-b rathod-b requested a review from Bill-Becker May 8, 2024 21:48
@rathod-b
Copy link
Collaborator Author

rathod-b commented May 8, 2024

Hi @Bill-Becker , we have two questions on this PR:

  1. if a portfolio is deleted on the UI, or if a run is unlinked from a portfolio, should we change the portfolio_uuid for associated runs to Null and unlink? It could be useful to have the portfolio_uuid in runs unlinked from a portfolio for future auditing purposes as needed.
  2. How should we handle updating the portfolio_uuid database field for portfolios that have already been created by users in production?

@rathod-b rathod-b changed the base branch from master to develop May 8, 2024 21:55
@Bill-Becker
Copy link
Collaborator

Hi @Bill-Becker , we have two questions on this PR:

  1. if a portfolio is deleted on the UI, or if a run is unlinked from a portfolio, should we change the portfolio_uuid for associated runs to Null and unlink? It could be useful to have the portfolio_uuid in runs unlinked from a portfolio for future auditing purposes as needed.
  • I'm not super clear on what is stored in the reopt-web db and how that changes when a portfolio is deleted or a run is unlinked. I assume there's no actual change to the API db for those runs. Are you asking if we should update the API db to remove/null the portfolio_uuid? I doubt we will do anything with that in the future, so I'd say just do whatever is easiest to fix the current multi-call issue.
  1. How should we handle updating the portfolio_uuid database field for portfolios that have already been created by users in production?
  • Maybe my response to 1 makes this clear, maybe not. This is referring to "how" we do 1, assuming we do actually want to update/null it, right? If you agree with my response in 1, then I think we just leave it and don't update it. If you think we should update it, then I think we need a new endpoint that would make that update.

@rathod-b
Copy link
Collaborator Author

Hi @Bill-Becker , we have two questions on this PR:

  1. if a portfolio is deleted on the UI, or if a run is unlinked from a portfolio, should we change the portfolio_uuid for associated runs to Null and unlink? It could be useful to have the portfolio_uuid in runs unlinked from a portfolio for future auditing purposes as needed.
  • I'm not super clear on what is stored in the reopt-web db and how that changes when a portfolio is deleted or a run is unlinked. I assume there's no actual change to the API db for those runs. Are you asking if we should update the API db to remove/null the portfolio_uuid? I doubt we will do anything with that in the future, so I'd say just do whatever is easiest to fix the current multi-call issue.
  1. How should we handle updating the portfolio_uuid database field for portfolios that have already been created by users in production?
  • Maybe my response to 1 makes this clear, maybe not. This is referring to "how" we do 1, assuming we do actually want to update/null it, right? If you agree with my response in 1, then I think we just leave it and don't update it. If you think we should update it, then I think we need a new endpoint that would make that update.

Sounds good on point 1. On point 2, i was referring to portfolio runs which have been executed since portfolio runs were made available in the API/webtool production and the day we deploy this feature branch. Those portfolio runs at present dont have a portfolio_uuid and should be assigned one for proper filtering in Django. So we may need a script for this.

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.

None yet

2 participants