[ENG-10338] Registrations not becoming public when embargo ends#11748
[ENG-10338] Registrations not becoming public when embargo ends#11748antkryt wants to merge 2 commits into
Conversation
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Looks good overall, there is some issue with my local admin app, so I merged into a temporary branch https://github.com/CenterForOpenScience/osf.io/tree/feature/hotfix-cr-test for me to test it.
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Left a few questions and suggestions @antkryt .
In addition, have you tested all three lists with pagination locally (not unit tests)? If not, please test it. You can tweak your pagination size to a lower value like 3 so you have less data to create.
If for some limitation, you can't test it locally or you can only test some of them, please document them.
(Note: the ticket is currently assigned to QA for creating registrations/embargoes on staging2.)
| def non_deleted_registations(self): | ||
| return self.filter(registrations__is_deleted=False).distinct() |
There was a problem hiding this comment.
A good fix. It reduces the query set and removed duplicates when filter across tables. Two questions:
- Can embargo has deleted registrations?
- Are there any other things we can filter out further?
In addition, rename non_deleted_registations to has_non_deleted_registations.
There was a problem hiding this comment.
- We can delete registration via admin or it can be marked as spam, etc. after embargo is created
- I think there's no need to filter out further. Functions like
pending_past_activation_windoware implemented to matchEmbargo.should_be_embargoedorEmbargo.should_be_completedfunctions which filter out only deleted registrations
| self.pending_embargoes() | ||
| .filter(initiation_date__lte=cutoff) | ||
| .non_deleted_registations() |
There was a problem hiding this comment.
Should we filter out embargoes with deleted registrations first?
There was a problem hiding this comment.
Yeah, I moved this filter to the pending_embargoes()/active_embargoes() as you suggested below
| self.pending_embargoes() | ||
| .filter(initiation_date__lte=cutoff) | ||
| .non_deleted_registations() |
There was a problem hiding this comment.
In addition, I recommend updating the pending_embargoes() directly
def pending_embargoes(exclude_deleted=false):
...so that we can simply call
return (self.pending_embargoes(exclude_deleted=true).filter(initiation_date__lte=cutoff))| def active_past_end_date(self): | ||
| """Active embargoes past end date (matches should_be_completed).""" | ||
| return ( | ||
| self.active_embargoes() | ||
| .filter(end_date__lt=timezone.now()) | ||
| .non_deleted_registations() | ||
| ) | ||
|
|
||
| def active_upcoming(self): | ||
| """Active embargoes with a future end date.""" | ||
| return ( | ||
| self.active_embargoes() | ||
| .filter(end_date__gte=timezone.now()) | ||
| .non_deleted_registations() | ||
| .order_by('end_date') | ||
| ) |
There was a problem hiding this comment.
Same question and suggestion for active_embargoes().
| return ( | ||
| self.pending_embargoes() | ||
| .filter(initiation_date__lte=cutoff) | ||
| .non_deleted_registations() |
There was a problem hiding this comment.
Finally, I see we have order_by() for active_upcoming(). So what's the consideration for no sorting for pending_past_activation_window and active_past_end_date while having sorting for active_upcoming().
There was a problem hiding this comment.
In the ideal word we don't want to see any embargoes in the pending_past_activation_window/active_past_end_date (or, at least, keep their number as low as possible), so I don't see any reason to order_by. Anyway, I added ordering just in case
| - active embargoes that are past their end date | ||
| - upcoming active embargoes | ||
| """ | ||
| EMBARGO_REPORT_PAGE_SIZE = 10 |
There was a problem hiding this comment.
Not a blocker but would be great for this to be a configurable different servers. On all the non-production servers I tested, the paging functionality doesn't show up due to lack of embargoes.
There was a problem hiding this comment.
moved to website/settings.py
2 participants
Ticket
Purpose
optimize embargo report
(Added by @cslzchen ) Here is the initial PR that implements the embargo report feature: #11637
Changes
Side Effects
QE Notes
CE Notes
Documentation