Feature month segregation for project report list by maciejSamerdak · Pull Request #138 · Code-Poets/sheetstorm

maciejSamerdak · 2019-04-15T15:55:46Z

project-report-list view now accepts a month and year parameters to separate reports by each month. A navigation bar is also introduced, containing next and previous button to navigate between adjacent months, as well as form allowing to select desired month and year to jump to.

Related #137

Do not delete this branch until #139 and #256 are merged

rwrzesien · 2019-04-24T23:52:08Z

@maciejSamerdak Same here, could add few words explaining what is it doing?

If I understood correctly, this should be a view for going through months back and forth and then it load report for that month?

If yes, then going from 01.2000 to 01.2099 would require 99 * 12 requests? Or can I go directly to the requested date?

maciejSamerdak · 2019-04-29T14:22:38Z

I updated the description. Navigation bar includes form allowing to go directly to specified month.

maciejSamerdak · 2019-04-29T14:30:11Z

employees/common/constants.py

+MONTH_NAVIGATION_FORM_MAX_MONTH_VALUE = 12
+MONTH_NAVIGATION_FORM_MIN_MONTH_VALUE = 1
+MONTH_NAVIGATION_FORM_MAX_YEAR_VALUE = 2099
+MONTH_NAVIGATION_FORM_MIN_YEAR_VALUE = 1990


I figured this could be the year CodePoets was founded, but I forgot to ask about it and shoot me dead guys, I couldn't remember for the life of me, which anniversary we had back in autumn (5th, I checked now...). Unless it's more sturdy to start from 2000?

rwrzesien · 2019-05-12T20:41:48Z

@maciejSamerdak

Please rebase this branch on master after merging base branch, there are currently too many things in this pull request that are not related so its hard to review.
I like the general idea, but how about simplifying the UX part and using configured datepicker, and using its callback (https://api.jqueryui.com/datepicker/#option-onChangeMonthYear) to load the requested month data using AJAX call and replace current data? I think this could result in having less code in the end. But I am not sure until you apply point 1.

rwrzesien · 2019-05-12T20:42:35Z

@maciejSamerdak Please apply #138 (comment) , lets start with making rebase to see only changes from this pull request.

maciejSamerdak · 2019-05-27T16:01:59Z

I've ran into multiple problems on frontend while trying to implement form-based month switch on UX side, as suggested by @rwrzesien. I've spent entire day and sadly, I couldn't come to any sensible solution, so I had to rollback to previous POST related solution.

I've refactored entire feature a bit, moving the entire functionality from an inclusion tag to a view mixin. The code is basically the same, both the feature and related tests, I just moved it all elsewhere. Still, I believe a complete re-review is necessary. The cons: rather than render the navbar template from inclusion tag, we have to remember to include it manually in target template. The pros: the code might be considered cleaner, since all we have to do now for a Django-based view to include the navbar, is to simply join the mixin

dybi

Please clarify.

employees/common/constants.py

dybi · 2019-05-31T08:33:44Z

employees/forms.py

+    )
+
+    def __init__(  # pylint: disable=keyword-arg-before-vararg
+        self, initial_date: datetime.date = None, *args: Any, **kwargs: Any


it is

Suggested change

self, initial_date: datetime.date = None, *args: Any, **kwargs: Any

self, initial_date: Optional[datetime.date] = None, *args: Any, **kwargs: Any

;)

Was this solution meant to silence pylint? Because it still cries about it. ☹️
Mypy doesn't mind either way...

well, it's weird - it looks like it doesn't complain on mine ;)
As to the root of the problem - initial_date is of Optional[datetime.date], because it can be None: https://mypy.readthedocs.io/en/latest/kinds_of_types.html#optional-types-and-the-none-type

employees/tests/test_unit_custom_mixins.py

employees/views.py

dybi · 2019-05-31T14:57:01Z

employees/tests/test_unit_custom_mixins.py

+        }
+        for key in expected_output.keys():  # pylint: disable=consider-iterating-dictionary
+            if key == "month_form":
+                self.assertTrue(expected_output[key].__eq__(context.dicts[1][key]))


why not self.assertEquals?

There's an issue with the MonthSwitchForm object. It's the only object that cannot be compared with == operator, self.assertEquals fails on this one.

the funny thing is that == is basically obj.__eq__(some_other_obj) ;) I checked it with debbuger: expected_output[key].__eq__(context.dicts[1][key]) returns NotImplemented which's value is different than False.

To sum up: blease implement __eq__ method in MonthSwitchForm and use normal assertEquals in the test ;)

dybi · 2019-05-31T14:57:58Z

employees/tests/test_unit_custom_mixins.py

+            "title_date": "03/19",
+        }
+        for key in expected_output.keys():  # pylint: disable=consider-iterating-dictionary
+            if key == "month_form":


Could you please explain this differentiation?

MonthNavigationText class under presented under navigation_text cannot be compared using 'eq', it causes an error in test (I guess it's because it's not an instance?).
MonthSwitchForm is the only object in the whole package that causes an issue on self.assertEqual(), therefore there's this differentiation.

please see above

employees/views.py

dybi · 2019-05-31T15:11:55Z

managers/views.py


+    def get_context_data(self, **kwargs: Any) -> dict:
+        context = super().get_context_data(**kwargs)
+        context["year"] = timezone.now().year


maybe a silly question, but why do we use timezone.now() here when in (almost?) all other cases we use datetime.now()?

Generally, I treat them interchangeably, since timezones are not a big deal for the most part. There was one instance when timezone had to be used over datetime in unit tests, because of issues with freezegun when comparing timestamps. Majority of files I've been working on were already using datetime, so I just stuck to it. Here, there was no such module before, so I picked timezone. I guess I just consider it more reliable. I also like the simpler syntax, instead of awkward double datetime.

Yeah, I guess I could just import date and datetime separately and refactor the existing code to avoid it...
Or perhaps, if we were to change the use of datetime in code, we should drop it altogether in favour of Django's timezone?

maybe we could refactor out code to use django's timezone, but I think it should be another issues.
@rwrzesien what do you think?

@dybi Yes, there was an issue for that I think to change all usages to timezone. Basically it does the same as datetime, but takes into consideration Django timezone settings (defined in settings file). So if at some point of the project we decide to use timezone(s) or change current timezone it is already done everywhere already. So I would say its just the matter of good Django practice.

rwrzesien · 2019-06-04T22:37:40Z

@maciejSamerdak

I've ran into multiple problems on frontend while trying to implement form-based month switch on UX side, as suggested by @rwrzesien.

I rather meant using JS datepicker + callback for loading other months data via AJAX call. This way you would just need few lines of JS datepicker setup and a view for returning month data. But you did a lot of work here so lets keep current approach.

kbeker

Code for me looks fine. I tested this manually and there everything working good as well. :)

maciejSamerdak added the feature New feature label Apr 15, 2019

maciejSamerdak added this to the Next release milestone Apr 15, 2019

maciejSamerdak requested review from Karrp, MartynaAnnaGottschling, Szymiks and kbeker April 15, 2019 15:55

maciejSamerdak self-assigned this Apr 15, 2019

This was referenced Apr 15, 2019

Feature hours per month counter #140

Merged

Feature month segregation for report list #139

Closed

maciejSamerdak assigned rwrzesien and kbeker Apr 15, 2019

maciejSamerdak requested a review from rwrzesien April 15, 2019 16:05

maciejSamerdak mentioned this pull request Apr 15, 2019

Feature hours per day counter #137

Merged

maciejSamerdak commented Apr 29, 2019

View reviewed changes

maciejSamerdak force-pushed the feature-hours-per-day-counter branch from 0acce45 to e97fc92 Compare May 7, 2019 12:45

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch from e11bafa to 12ddcf0 Compare May 7, 2019 12:45

maciejSamerdak force-pushed the feature-hours-per-day-counter branch from e97fc92 to 78ec9d6 Compare May 10, 2019 15:48

maciejSamerdak mentioned this pull request May 14, 2019

Feature distribution of reports for months #25

Closed

3 tasks

maciejSamerdak force-pushed the feature-hours-per-day-counter branch from 78ec9d6 to 23c5f8b Compare May 14, 2019 11:48

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch from 12ddcf0 to 1473968 Compare May 14, 2019 15:48

kbeker force-pushed the feature-hours-per-day-counter branch from 855cf27 to 7d968fb Compare May 15, 2019 10:20

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch 2 times, most recently from 497584e to f3ddaa3 Compare May 16, 2019 17:05

maciejSamerdak force-pushed the feature-hours-per-day-counter branch from 1984cd3 to b400b27 Compare May 16, 2019 17:06

Szymiks mentioned this pull request May 17, 2019

Feature add export reports to xlsx #212

Merged

1 task

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch from f3ddaa3 to f7c41c9 Compare May 17, 2019 16:34

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch from f7c41c9 to 866802c Compare May 24, 2019 12:27

kbeker added this to the v0.3.0 milestone May 27, 2019

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch 2 times, most recently from 916b585 to 1fbfd2c Compare May 27, 2019 15:53

maciejSamerdak changed the base branch from feature-hours-per-day-counter to master May 27, 2019 15:54

maciejSamerdak requested a review from dybi May 29, 2019 09:16

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch 2 times, most recently from a02b686 to 9c162fc Compare May 29, 2019 11:07

maciejSamerdak modified the milestones: v0.3.0, Next release May 29, 2019

maciejSamerdak mentioned this pull request May 29, 2019

Feature month segregation for author report list #256

Merged

kbeker removed this from the Next release milestone May 30, 2019

dybi reviewed May 31, 2019

View reviewed changes

maciejSamerdak force-pushed the feature-month-segregation-for-project-report-list branch 2 times, most recently from efc1671 to 8b0dd7b Compare June 4, 2019 16:43

rwrzesien approved these changes Jun 4, 2019

View reviewed changes

kbeker approved these changes Jun 5, 2019

View reviewed changes

maciejSamerdak added 7 commits June 5, 2019 07:03

Add constants for month navigation form's value limits

3a55a47

Add form for switching months in report list view

25b6edb

Add string constants for month navigation's UI text

cb7ae9d

Add template for month navigation bar

1e61555

Update project report list view for month segregation

58f1997

Update link in managers app

71cbb66

Add feature related unit tests

ac80ae8

kbeker force-pushed the feature-month-segregation-for-project-report-list branch from 8b0dd7b to ac80ae8 Compare June 5, 2019 05:03

kbeker merged commit ac80ae8 into master Jun 5, 2019

kbeker added this to the next_release milestone Jun 5, 2019

kbeker deleted the feature-month-segregation-for-project-report-list branch June 5, 2019 07:41

	self, initial_date: datetime.date = None, args: Any, *kwargs: Any
	self, initial_date: Optional[datetime.date] = None, args: Any, *kwargs: Any

Conversation

maciejSamerdak commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwrzesien commented Apr 24, 2019

Uh oh!

maciejSamerdak commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwrzesien commented May 12, 2019

Uh oh!

rwrzesien commented May 12, 2019

Uh oh!

maciejSamerdak commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dybi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejSamerdak May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejSamerdak May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwrzesien commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maciejSamerdak commented Apr 15, 2019 •

edited

Loading

maciejSamerdak commented May 27, 2019 •

edited

Loading

maciejSamerdak May 31, 2019 •

edited

Loading

maciejSamerdak May 31, 2019 •

edited

Loading

rwrzesien commented Jun 4, 2019 •

edited

Loading