added Yearly Reading Goals to /stats page

[kevinc16]

Closes #7352

Added summary and total_yearly_reading_goals functions to yearly_reading_goals.py. Added unit test for total_yearly_reading_goals function. Added Yearly Reading Goals row in openlibrary/templates/admin/index.html.

Let me know if there are things we want to change!

Technical

There are a few things to note:

1. First, during testing, I found that for some reason the key in the storage object (noted by @scottbarnes in 7352/yearly reading goal stats #7560 (comment), Scott is also right that the other functions are documented incorrectly - they return Storage objects not ints) in the unit test is different from when the application is run (that is the reason in the HTML file the key is 'count' while in the unit test it is 'count(*)')

Application log:

web-1           | total_yearly_reading_goals (<Storage {'count': 1}>,)
web-1           | total_yearly_reading_goals (<Storage {'count': 1}>,)
web-1           | total_yearly_reading_goals (<Storage {'count': 1}>,)

But when I use the key 'count' in the unit tests:

====================================================================================== FAILURES ======================================================================================
_______________________________________________________________ TestYearlyReadingGoals.test_total_yearly_reading_goals _______________________________________________________________
self = <core.test_db.TestYearlyReadingGoals object at 0x7f8426bf9dc0>

    def test_total_yearly_reading_goals(self):
>       assert BookshelvesEvents.total_yearly_reading_goals()['count'] == 3
E       KeyError: 'count'

openlibrary/tests/core/test_db.py:523: KeyError
-------------------------------------------------------------------------------- Captured stdout call --------------------------------------------------------------------------------
total_yearly_reading_goals (<Storage {'count(*)': 3}>,)

2. The second is that I noticed the other stat functions (e.g., total_books_logged in openlibrary/core/bookshelves.py) don't have unit tests - is there a specific reason for that?

Testing

UI changes and docker compose run --rm home make test.

Screenshot

Stakeholders

@jimchamp

[jimchamp]

Thanks @kevinc16! This all seems to be working as expected. The only problem is that we asked you to place the new yearly_reading_goals DB functions in the wrong file. Could you move those functions to this file and update their references?

[jimchamp]

Could you move this and the total_yearly_reading_goals function to https://github.com/internetarchive/openlibrary/blob/master/openlibrary/core/yearly_reading_goals.py?

[jimchamp]

After moving this function to core/yearly_reading_goals.py, please replace the hard-coded table name with TABLENAME:

openlibrary/openlibrary/core/yearly_reading_goals.py

Line 6 in 112cf14

TABLENAME = 'yearly_reading_goals'

[jimchamp]

Suggested change

	results = tuple(oldb.query(query, vars={'since': since}))
	print('total_yearly_reading_goals', results)
	results = list(oldb.query(query, vars={'since': since}))

While casting to a tuple works here, please cast to a list instead. This is how it's done throughout the codebase, and I'm not sure if using tuple here may have unintended side-effects.

Also, remove the print statement.

[jimchamp]

The second is that I noticed the other stat functions (e.g., total_books_logged in openlibrary/core/bookshelves.py) don't have unit tests - is there a specific reason for that?

We simply don't have good test coverage.

[kevinc16]

The second is that I noticed the other stat functions (e.g., total_books_logged in openlibrary/core/bookshelves.py) don't have unit tests - is there a specific reason for that?

We simply don't have good test coverage.

I see - do you think you think we should make an issue for that? I wouldn't mind adding a few more unit tests (for the related functions on the stats page - and also updating the comments 😁).

Also, in terms of the 1st point I raised in my PR, is there anything that should be noted there?

[jimchamp]

Thanks again. @kevinc16! Just make sure that totaly_yearly_reading_goals returns an int, and remove the unit tests. Due to the way that these DB unit tests are currently set up, some functions cannot be tested.

[jimchamp]

Suggested change

	results = list(oldb.query(query, vars={'since': since}))
	return results[0]
	results = oldb.query(query, vars={'since': since})
	return results[0]['count'] if results else 0

Just return an int here. This follows the same pattern used in Booknotes.total_booknotes.

[jimchamp]

Suggested change

	def total_yearly_reading_goals(cls, since: date | None = None) -> int:
	"""Returns a Storage object of <Storage {'count': int}> where 'count' specifies the
	number reading goals updated. `since` may be used
	def total_yearly_reading_goals(cls, since: date | None = None) -> int:
	"""Returns the number reading goals that were set. `since` may be used

Type hints and docstring are conflicting. Return an int here.

[jimchamp]

Suggested change

	def test_total_yearly_reading_goals(self):
	assert YearlyReadingGoals.total_yearly_reading_goals()['count(*)'] == 3
	# Combination of issues
	assert (
	YearlyReadingGoals.total_yearly_reading_goals(since="2022-12-25")[
	'count(*)'
	]
	== 1
	)

Just remove this test. The DB used for this test is not mocked --- we spin up an instance of sqlite for these tests, but we use Postgres in production. It looks like the key for the counts is different depending on which DB is used.

[kevinc16]

Ah I see, that explains why they are different in the 2 places - but if that is the case, why don't we just mock the Postgres DB? Is there a specific reason?

[jimchamp]

If you want to add more unit tests, create a feature request proposal issue and outline what test you will be adding.