-
Notifications
You must be signed in to change notification settings - Fork 267
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
Display monthly statistics for the range of months where the project was active #885
Conversation
8af4692
to
89c4875
Compare
I'm not sure : should we get a test for this? |
Hmm, yes, it could be useful |
Hey @zorun, here's a gentle ping on this so we can merge it before it rots (if you have the time, of course!). |
I don't have time to add tests for this anytime soon. You decide if it can be merged without tests or not :) |
89c4875
to
0c81b2c
Compare
Rebased. |
I'm currently adding a test, and it is catching a corner case I had overlooked, so it was definitely a good suggestion, thanks @almet 👍 |
0c81b2c
to
3222a44
Compare
b5d5f7a
to
6c2cb73
Compare
@almet I took into account your remarks, fixed the corner cases, and added a test. A second quick review would be appreciated, thanks! |
6c2cb73
to
aa5d51e
Compare
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.
Hey, here's a quick review. Overall, I think we're good and I'm okay merging it as-is. That being said, if you want to polish things a bit more I've left a few comments. Don't hesitate to ignore them if you feel I'm too picky.
ihatemoney/tests/budget_test.py
Outdated
months_fmt = [ | ||
r"<tr>\s*<td>{}</td>\s*<td>{}</td>\s*</tr>".format(month, amount) | ||
for month, amount in months | ||
] |
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.
Actually, I find it kinda weird to check the output of the HTML like this, because it means that if we change the format of the table we also need to change the tests, even though we're not changing the actual behavior of the code.
I wonder if we might instead create a model function, and test that it works properly with python-related tests (more unit tests than functional tests, though). We might also want to have this information on the /api
endpoint.
What do you think?
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 must admit that I'm not completely happy with this regexp magic ;) and when the test fails, it's really hard to know why, you just get a big blob of HTML soup and you have to figure out what's wrong in it.
I hadn't thought of testing the function itself, but now that you mention it, it's obviously a good idea... I guess I just continued what the existing test was doing.
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.
As explained below, I just implement this new testing approach!
For the API, I'm not sure. The client could just request the list of bills and compute any statistics itself.
This makes it easier to use datetime.date later.
…was active Currently, we display a hard-coded "one year" range of monthly statistics starting from today. This generally is not the intended behaviour: for instance, on an archived project, the bills might all be older than one year, so the table only displays months without any operation. Instead, display all months between the first and last bills. There might be empty months in the middle, but that's intended, because we want all months to be consecutive. If there are no bills, simply display an empty table.
aa5d51e
to
af2a711
Compare
I have changed the approach a bit:
@almet I guess another review is in order :) |
It's far better this way, thanks! |
…was active (spiral-project#885) * Change the way we import datetime This makes it easier to use datetime.date later. * Display monthly statistics for the range of months where the project was active Currently, we display a hard-coded "one year" range of monthly statistics starting from today. This generally is not the intended behaviour: for instance, on an archived project, the bills might all be older than one year, so the table only displays months without any operation. Instead, display all months between the first and last bills. There might be empty months in the middle, but that's intended, because we want all months to be consecutive. If there are no bills, simply display an empty table. Co-authored-by: Baptiste Jonglez <git@bitsofnetworks.org>
Currently, we display a hard-coded "one year" range of monthly statistics
starting from today. This generally is not the intended behaviour: for
instance, on an archived project, the bills might all be older than one
year, so the table only displays months without any operation.
Instead, display all months between the first and last bills. There might
be empty months in the middle, but that's intended, because we want all
months to be consecutive.
If there are no bills, simply display an empty table.