Bugfix variable default date value on report creation#375
Conversation
maciejSamerdak
requested review from
MartynaAnnaGottschling,
Szymiks,
dybi,
kbeker and
rwrzesien
maciejSamerdak
force-pushed
the
bugfix-default-date-in-different-months
branch
from
2ae98e9 to
0bc40d7
Compare
|
Now that I think of this, most people might be filling multiple reports on same day. That means the one day after the last report default might be counter-intuitive. What do you think of this? |
|
@maciejSamerdak This plan looks good, only one thing I think you could remove point (1) and apply (3) to current month too. It would be most often case that someone fills reports for current month. Also, you should move this description to original issue. |
employees/views.py
Outdated
| return datetime.date(year=year, month=month, day=1) | ||
| date = queryset.first().date + timezone.timedelta(days=1) | ||
| if date.month != month: | ||
| return queryset.first().date |
There was a problem hiding this comment.
You should evaluate queryset only once.
There was a problem hiding this comment.
I forgot this would be an issue. At this point I treated it more like list-based structure returning reference to it's head.
Is it enough to reduce .first() calls to one or should I maybe call .first() on the get_queryset() and call .exists() on that?
There was a problem hiding this comment.
Not big issue, but rather optimization. The way you did it now is OK.
|
@rwrzesien So for all months it will behave the same? As in: if there are no reports in current month then the date is the first day? It might work most of the time, except for few instances. But same can be said about the current date. I think @Karrp should give some insight on which option he would find preferable. |
Yes, this is what I have meant. This is just my opinion based on my experience in adding reports via Sheetstorm. I don't think I have added the report for previous month so far, and it would be great if I have the date set to next day from last report in current month. But I understand it won't be best solution for everyone. |
How about taking creation date under consideration? If the last report from the month wasn't filled today, the default date will be the next day. But if the last report was filled today, then default date is last report's date. So if we display any list and we haven't edited it today, we get the next day (for example: today, if it's the current month and we filled some reports yesterday). Then once we add new report, the date remains the same, allowing us to conveniently fill out other reports from this single day, without constantly changing the date one day back. This will work like a charm, if we want to fill in today's report or if we want to fill last reports from previous month. Seems like most common use cases. |
Applied changes described in #375 (comment)
Applied changes described in #375 (comment)
|
Recent fixups apply changes I described in the previous comment. |
Makes sense, I think it is good trade of between moving to next date and staying on the same day. |
kbeker
force-pushed
the
bugfix-default-date-in-different-months
branch
from
bc85e09 to
bb237dd
Compare
kbeker
changed the title
3 participants
Resolves #366
Added method for deriving default date value in report creation form. This is how it works:
This can be useful if the user is late on filling reports. The date on previous months defaults to the day after the last report (if possible), so after each post the default date value is raised by one day.
Done: