-
Notifications
You must be signed in to change notification settings - Fork 351
[ENG-9122] Fix/eng 9122 #11435
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
[ENG-9122] Fix/eng 9122 #11435
Conversation
cslzchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall and left a few questions for me to understand the issue and the fix better.
osf/models/preprint.py
Outdated
| first_version = base_guid.versions.filter(is_rejected=False).order_by('version').first() | ||
| first_version = base_guid.versions.order_by('version').first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's the reason behind this change? Does it have to do with the corner case where all versions are rejected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the filter(is_rejected=False) ensures:
- We always get the actual first version chronologically (version 1)
- The code handles the "all versions rejected" case correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see date_created_first_version is a new property in the target pb&s feature branch. And thus this first_version here only affects this new property.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def reindex_versioned_preprints(dry_run=False, batch_size=100, provider_id=None, guids=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a "backfill" command that fix existing preprints' indexing. Do we have to run this daily (or more frequently) to re-index? If understand this correctly, we should/must also fix the indexing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, new preprints and versions are indexed appropriately.
cslzchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the explanation. Make sure @adlius and/CE is aware of the command that needs to be run in each server.
osf/models/preprint.py
Outdated
| first_version = base_guid.versions.filter(is_rejected=False).order_by('version').first() | ||
| first_version = base_guid.versions.order_by('version').first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see date_created_first_version is a new property in the target pb&s feature branch. And thus this first_version here only affects this new property.
ef49be6
into
CenterForOpenScience:feature/pbs-25-21
Purpose
Update Created field in search card
Changes
Add new propery for Preprint model date_created_first_version. As it propery reindex via admin page doesn't have efect, because date_created_first_version doesn't fit for save_fieds. So I add manage command to reindex all existing preprints properly.
QA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket
https://openscience.atlassian.net/browse/ENG-9122?atlOrigin=eyJpIjoiMWFlODI2MzVjZDI5NDkyY2I2M2FhYzc3NDdhZGM0NmEiLCJwIjoiaiJ9