[ENG-10338] Registrations not becoming public when embargo ends#11748
[ENG-10338] Registrations not becoming public when embargo ends#11748antkryt wants to merge 1 commit 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.
| 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?
| 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().
| - 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.
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