Suppression state propagation #328
Conversation
…es (e.g. Study) to descendant data types (e.g. Analysis)
There was a problem hiding this comment.
This is great stuff @SandyRogers . I've added 2 minor comments. I think @MGS-sails should give this PR a look before merging.
Co-authored-by: Martín Beracochea <[email protected]>
| self.is_suppressed = True | ||
| self.suppressed_at = timezone.now() | ||
| self.suppression_reason = suppression_reason | ||
| if save: | ||
| self.save() | ||
| if propagate: |
There was a problem hiding this comment.
@SandyRogers might it be better to return here, if not propagate ?
This might help to reduce nesting inside the if block and the for loops inside it.
| self.is_suppressed = False | ||
| self.suppressed_at = None | ||
| self.suppression_reason = None | ||
| if save: | ||
| self.save() | ||
| if propagate: |
There was a problem hiding this comment.
@SandyRogers this might be overkill, and please ignore if it's going to cause an uneccessary delay,
but I think in the long term, it will be better to refactor the suppress and unsuppress methods.
It seems like they are doing the same thing, with only slight variations.
So it might be better if they both call a generalised method, but with different arguments.
|
@SandyRogers should we merge this one? |
…tion' into suppression-state-propagation # Conflicts: # emgapi/models.py
This PR:
select_related_onlyqueryset method, to reduce the amount of data selected by complex queries like theAnalysisJob.study.accessionfor ananalysisjob), without querying the many other fields onsamplethat are never picked by the serializer.Together these simplify the querying of some models, because suppression states of related objects do not have to be checked at query time and a slightly smaller query is used. This has been implemented on the Analysis endpoint currently, and seems to give a speedup of around 20%... to get more improvement, we need to do something similar with the
is_privatestates. These are more complicated because we need to addsubmission_accountinfo on other models, and check this depending on the request user. I have not done that in this PR, but it might be worth a go in future...