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

Bug fix and cover a gap #601

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Bug fix and cover a gap #601

merged 3 commits into from
Sep 25, 2024

Conversation

rathod-b
Copy link
Collaborator

@rathod-b rathod-b commented Sep 24, 2024

Fix a few bugs and cover a gap in re-linking portfolios and runs.

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, ...)
Bug fix

What is the current behavior?

(You can also link to an open issue here)
Runs cannot be unlinked; once a run is unlinked from a portfolio, it cannot be re-linked to that portfolio.

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

Runs can be unlinked and successfully relinked to portfolios

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
No

Other information:

Fix a few bugs and cover a gap in re-linking portfolios and runs.
@@ -734,12 +734,23 @@ def link_run_uuids_to_portfolio_uuid(request):
for s in scenario:
s.portfolio_uuid = p_uuid
s.save()

Copy link
Collaborator Author

@rathod-b rathod-b Sep 24, 2024

Choose a reason for hiding this comment

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

@Bill-Becker Am I allowed to delete PortfolioUnlinkedRuns objects?

I realized that when we link a portfolio to an existing run, there can be a situation where the run is listed in PortfolioUnlinkedRuns, ie it was unlinked from a portfolio previously. In this case we do not want the run to show up in independent summary endpoint runs, which is why we'd have to delete that entry from PortfolioUnlinkedRuns table. I tested to see that an existing run can be deleted from PortfolioUnlinkedRuns so it doesnt show up in independent summary runs, and then unlinked from that portfolio so it does show up. This could be a niche use case so i can remove this.

@@ -810,6 +821,8 @@ def summary(request, user_uuid):
run_uuid__in=[i.run_uuid for i in UserUnlinkedRuns.objects.filter(user_uuid=user_uuid)]
).only(
'run_uuid',
'user_uuid',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added these two fields in Summary response for informational and validation purposes. The UI can ignore these.

@@ -1287,7 +1302,7 @@ def unlink_from_portfolio(request, user_uuid, portfolio_uuid, run_uuid):
elif runs[0].user_uuid != user_uuid:
return JsonResponse({"Error": "Run {} is not associated with user {}".format(run_uuid, user_uuid)}, status=400)
else:
return JsonResponse({"Error": "Error in unlinking run {} from portfolio {}".format(run_uuid, portfolio_uuid)}, status=400)
pass
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed a bug here so unlinking actually happens.

Updated JSON response to delineate between adding a portfolio uuid vs deleting from existing portfolio runs.
@rathod-b rathod-b marked this pull request as ready for review September 25, 2024 20:37
@rathod-b rathod-b merged commit 0927396 into develop Sep 25, 2024
2 checks passed
@rathod-b rathod-b deleted the summary-endpoint-updates branch September 25, 2024 21:05
@rathod-b rathod-b mentioned this pull request Sep 26, 2024
3 tasks
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.

2 participants