refactor: analytics into atomic operations

[brunocorrea23]

targets #294

[iuioiua]

Great! Thanks for this. Please also check that their respective functions have increased the analytics data. E.g. that users_count has been incremented in the createUser() test.

Can we also investigate removing incrementAnalyticsMetricPerDay()?

[brunocorrea23]

Thanks!

• I have added tests for the analytics inside the tests for createUser() and createItem(). I didn't do it for createVote() since this test has not been created yet and is part of an ongoing PR. We can add that later.
• I've removed the incrementVisitsPerDay() in favor of a more generic incrementAnalyticsMetricPerDay() that can be reused if more cases in the future. Also did some refactoring around that. Let me know if this makes sense. I can undo these changes if it doens't.

[iuioiua]

IMO, we should drop getAnalyticsMetricPerDay() in favour of, say getItemsCountByDate(), and the same for the others. I'd rather have more lines of code that are easier to understand rather than fewer lines of code that require more thinking. WDYT?

[iuioiua]

Hm... It might be better to define a const usersCount at the start of the function with the others without using analyticsKey(). It gives a more immediate idea of what's going on, IMO. At most, maybe a toDate() that returns date.toISOString().split("T")[0] might be useful. Ditto for other similar scenarios. WDYT?

[iuioiua]

I.e. check out formatDate() in #289.

[brunocorrea23]

Makes sense. Will adjust that!

[iuioiua]

Suggested change

	const itemsCount =
	(await getAnalyticsMetricPerDay("items_count", new Date()))?.valueOf() ??
	0n;
	const itemsCount = await getAnalyticsMetricPerDay("items_count", new Date());
	assertEquals(itemsCount, null);

[brunocorrea23]

@iuioiua , can you pls explain this suggestion here?
I believe that if you do it like this, the assertion is going to fail. When it gets to this point, the itemsCount is probably equal to 2n because of the two createItem() that were run on the test above (" [db] getAllItems()")

We can't assume anything about the value o itemsCount, as we don't know how many tests will use it before in the code. We can only test that the itemsCount_after = itemsCount_before + 1.

Does it make sense? Did I understand correctly your suggestion?

[iuioiua]

Perhaps, we should move the line that defines itemsCount elsewhere such that it doesn't require a default value. WDYT?

[brunocorrea23]

IMO, we should drop getAnalyticsMetricPerDay() in favour of, say getItemsCountByDate(), and the same for the others. I'd rather have more lines of code that are easier to understand rather than fewer lines of code that require more thinking. WDYT?

Thanks for your input here 🙏 I'll do some refactoring around this so that we end up with 3 functions per metric:

• increment[X]ByDay(date: Date)
• get[X]ByDay(date: Date)
• get[X]ListByDay(options?: Deno.KvListOptions)

[brunocorrea23]

@iuioiua , couple of comments here:

• I've created the analytics functions I've mentioned above.
• I haven't changed the way /stats uses the analytics metrics as I believe this is outside the scope of this PR and might overlap with the work on fix(stats): populate empty chart axis #289 . If the changes are not handled by that PR, I can create another one soon after it.
• As for the default value on the db_test.ts you've mentioned above: the way I see to remove the need for a default value is simply to move the function to the top of the file, where we can guarantee that no one has increased that metrics count before. But I do think that the current implementation is more robust, as it doesn't assume anything about order of functions within that file. I can imagine in the future someone trying to changing that file, the test starts failing and it will be hard to debug why.

[iuioiua]

Few nits. Could you please also rebase? Sorry, I only just pushed a DB-related change.

• As for the default value on the db_test.ts you've mentioned above: the way I see to remove the need for a default value is simply to move the function to the top of the file, where we can guarantee that no one has increased that metrics count before. But I do think that the current implementation is more robust, as it doesn't assume anything about order of functions within that file. I can imagine in the future someone trying to changing that file, the test starts failing and it will be hard to debug why.

Well-justified. Let's go with your suggestion then.

[brunocorrea23]

Addressed the previous comments. Please take a look.

[iuioiua]

Great work! Nice cleanups and simplifications! Thanks for this one, @brunocorrea23.

[iuioiua]

Closes #294

[brunocorrea23]

@iuioiua , thank you for all the help and input! 🙏