-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding author date issue7799 #8076
Adding author date issue7799 #8076
Conversation
for more information, see https://pre-commit.ci
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.
OPTIONAL: I see that we are doing two things in multiple places:
- get_date_from_string
- validate_author_dates
These could be made into separate functions...
begin_of_date = title.find('(') | ||
if begin_of_date: | ||
begin_of_date += 1 | ||
end_of_date = title.find(')') | ||
date = title[begin_of_date:end_of_date] |
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.
It would be cool to create a separate function from these lines...
def get_date_from_string(string):
<this code here>
return string[begin_of_date:end_of_date]
len_if_no_birth_data = 3 | ||
len_if_author_alive = 7 | ||
len_if_author_dead = 11 | ||
for date in author_dates: | ||
assert ( | ||
len(date) == len_if_no_birth_data | ||
or len(date) == len_if_author_alive | ||
or len(date) == len_if_author_dead | ||
) |
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 could be a function called...
def validate_author_dates(author_dates):
<this code here>
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.
Sorry for the delay @Lux-Sales ! It seemed like this PR got stuck.
I left a code review because I know @Aeltumn wanted to continue from this work. @Aeltumn , @jimchamp will help you branch off the changes here! Then you can address the comments on your new PR.
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.
We only want the dates to appear on the book page, not in the search result cards, so we shouldn't need any change to this file.
@@ -324,6 +324,8 @@ def get_doc(doc: SolrDocument): | |||
key=key, | |||
name=name, | |||
url=f"/authors/{key}/{urlsafe(name or 'noname')}", | |||
birth_date=doc.get('birth_date', 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.
Ditto here, this only applies to search pages.
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.
Also no need for the dates on the loans page here.
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.
We've decided we currently don't support integration tests, since our integration had been inactive for a very long time and were very few. So we don't need these integration tests.
@@ -1,21 +1,26 @@ | |||
$def with(author_names_and_urls, limit=None, overflow_url=None, attrs='') | |||
$def with(author_names_url_birth_date_death_date, limit=None, overflow_url=None, attrs='') |
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 should only optionally display the birth/death; e.g. show_years=True/False
.
(Note: Because we only want to include the years on books pages, the librarian-check should happen there, not in this template).
$ birth_date_formatted = '' | ||
$ death_date_formatted = '' | ||
$if birth_date and death_date: | ||
$ birth_date_formatted = birth_date[-4:] |
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.
We should be using a year regex here, since our dates are sometimes in weird formats
@@ -1,21 +1,26 @@ | |||
$def with(author_names_and_urls, limit=None, overflow_url=None, attrs='') | |||
$def with(author_names_url_birth_date_death_date, limit=None, overflow_url=None, attrs='') |
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 tuple is becoming a little large! I guess we should update this to pass in a dict at this point. So a dict with 4 fields, the author name, author url, birth date, death date.
Merging this into |
Closes #7799
[feature]
Technical
Now the birth date and the death date is show with the author name
Testing
Open any book with authors, if there's birth date and death date data, it will be displayed, if not besides the name will be just a ( - )
Screenshot
Stakeholders