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

Tadhg/cogassign no signals #2616

Merged

Conversation

tadhg-ohiggins
Copy link
Contributor

@tadhg-ohiggins tadhg-ohiggins commented Oct 25, 2023

Try the direct way
And not the indirect way;
Let’s see what happens.


Since one of the things I had to do was merge main into the my branch, in order for this PR to be reasonable you probably need to merge main into jmm/resolve-cog-baseline-bug.

I commented on 2483 suggesting something similar to this approach. This removes reliance on signals entirely and instead makes any save() of CognizantAssignment check to see if this is new object creation, and if it is, do the things that were formerly in support.signals.

@JeanMarie-PM
Copy link
Contributor

@tadhg-ohiggins Too many changes to fully check.
But I'm good with moving the code from signals.py to models.py.
I don't know where the call to cog is being made upon submission - hopefully, you've removed sac.sace() from there.
If all tests are passing, you have my approval to merge.

@tadhg-ohiggins
Copy link
Contributor Author

@JeanMarie-TTS now that main has been merged in, this PR consists of two changed files, one of which is signals.py being deleted entirely.

The change this branch makes is to override the save method of CognizantAssignemt so that whenever it’s invoked, it checks to see if the save is an instance being created or updated. If it’s being created, it then updates the corresponding SingleAuditChecklist appropriately and saves it, then also runs reset_baseline and attempts to update the corresponding dissemination.General entry as well (if one exists).

The logic is intended to be the same as was happening in support/signals.py. With this version, the code is synchronous and the code path more linear.

If the logic in this PR matches what you expect, you can approve it and merge it into jmm/resolve-cog-baseline-bug, and then if the tests on #2483 pass that should be good to merge into main.

@tadhg-ohiggins tadhg-ohiggins mentioned this pull request Nov 8, 2023
12 tasks
@JeanMarie-PM JeanMarie-PM merged commit f3299f3 into jmm/resolve-cog-baseline-bug Nov 8, 2023
@JeanMarie-PM JeanMarie-PM deleted the tadhg/cogassign-no-signals branch November 8, 2023 15:14
github-merge-queue bot pushed a commit that referenced this pull request Nov 8, 2023
* Fix bug

* lint

* Move signals functionality to models.py in support. (#2616)

* rm refs to signals

---------

Co-authored-by: Tadhg O'Higgins <[email protected]>
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