-
Notifications
You must be signed in to change notification settings - Fork 67
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
1322 most downloaded article #1344
Conversation
Closes #1344 1322 |
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.
Looking good.
Couple of comments inline
Main comment would be about moving the "most popular" feature to a plugin of its own.
self.fields['most_downloaded_time'].initial = most_downloaded_time | ||
|
||
most_downloaded = forms.BooleanField( | ||
label='Display Most Downloaded Articles', |
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.
Should this say, most popular or most visited since we are not filtering downloads but general accesses?
(same applies to the references to "downloads" below
help_text='Determines how many popular articles we should display.', | ||
) | ||
|
||
def save(self, journal, commit=True): |
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.
I don't think we should override the signature of save
to take in a journal. Especially since you already expect a journal to be passed to the constructor
request.journal | ||
) | ||
|
||
if most_downloaded: |
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.
this means we can't have both most downloaded and featured articles at the same time.
Perhaps the "most downloaded" should be a homepage element of its own, so you can choose to have, neither, one, the other or both
* Partial commit. * Form now works as expected. * Initial pass over. * Changed title on OLH theme * Switched to Processed Values. * Fixed query. * Partial: adds dt filtering. * openlibhums#1322: Use natural join filters instead of subquery * When articles have the same number of accesses, order by title. * Brought most popular articles to Default and Material. * Reset featured. * Resets featured templates. * Added new popular homepage element.
No description provided.