We shouldn't try and collect data from the future #42
Conversation
This logic took me ages to work out so I've added some extremely detailed documentation to the fixture. This is not the right thing to do or the right place to do it - the code and tests should be refactored so this logic is clear from reading the tests; however, I do not want to make too many changes at once so this will do for now.
As in, this is the result I expect
I can't see why this +1 has been added. The work it is doing means we include the current time period. Pending more complex logic for handling this (to allow users to enter data for previous time periods but not current time period) I do not think we should collect data for the current time period.
These three were failing because the latest time period was not being returned. Have fixed them therefore by setting the start date one day earlier.
Fixed a couple more tests by setting the start date to yesterday. The sceond one here, test_record_get now may not quite make sense because how can you record data if it's not on the view screen? But I don't think this matters right now.
...but there is probably a better way of doing this. I think it still tests what it's meant to test, but you might want to take a look.
|
This looks good to me; the tests still all pass, although for other reasons (mostly attempts to eschew VirtualBox, which turns out to be more problematic than using it) I cannot do anything serious via a local web instance either. I don't have a huge amount of time right now, so I won't actually merge it as I don't have time to write up the other ticket that needs to come out of it. If no one else gets to it by early next week, when I should have some more time, then I'm happy to spend a couple of hours getting a VBox working for this again, then doing all the admin and making this live. |
|
I raised the issue as #43. Would still appreciate another eye on this - maybe someone who can get it running locally? Ta. |
|
Cool — for some reason comments on commits go to one inbox, but issue stuff goes to another, so I didn't notice your issue. Sigh. I've spent the day upgrading my hosted server; hopefully I'll get some time tomorrow to wrestle VBox into submission and have a look at this. |
We shouldn't try and collect data from the future I have finally got behabitual working locally and it works fine, so I'm going to merge and deploy this change. Yes, merging my own pull requests. http://wheningit.tumblr.com/post/25796306185/when-i-merge-my-own-pull-requests
This pull request addresses issue #40. I hadn't quite understood the problem when I raised the issue so to restate it here:
Be Habitual requests information for the current time period. This is not a major problem if the time period is a day, but if it's a week it means you get an email at 9.30 on Monday morning asking you how you have done, and the link takes you to a page which wants the upcoming week's data (i.e. future data).
This is nonsensical, but it's also problematic in that you are not permitted to enter only some data on that page. So if I want to enter last week's data only, I can't, I also need to enter data for this week, which hasn't happened yet. So I have to wait until the end of this week to enter last week's data, or set this week's data forever to 0.
(a) having to enter the current period's data
(b) not being able to enter the current period's data
Currently, it is (a). This leads to the non-sensical situation I described above. Therefore, the changes in this PR changes the logic so it defaults instead to (b).
It looks like the +1 is solely to ensure we include the current time period.
Paricularly @nickstenning and @ashb who worked on this file; and/or @georgebrock, @mnowster, @SteveMarshall, @jaylett, @jcoglan, or @norm.
Oh, and the other thing I haven't done is tested it locally as my local running Be Habitual falls over when I try and create a habit (on master as well as this branch). Tips on that welcome...
If this is merged whoever does that could also raise an issue for point 1?
Cheers! To Industry!