Bugfix incorrect hours stats by maciejSamerdak · Pull Request #559 · Code-Poets/sheetstorm

maciejSamerdak · 2020-05-31T19:32:57Z

The whole way work hours summaries are calculated has been refactored. Mixin is no longer responsible for handling time filtering, since it's already performed on delivered querysets by corresponding views.
Additional unit tests, where a massive amount of reports is generated, have been added.

kbeker

There are few things to improve. If you will have any doubts just ask me :)

employees/tests/test_views.py

kbeker · 2020-06-09T05:59:09Z

employees/tests/test_views.py

+        self.assertTrue("projects_work_percentage" in context_data.keys())
+
+        projects_work_percentage = context_data["projects_work_percentage"]
+
+        expected_result = {
+            project: ([work_hours_sum, work_hours_sum / self.total_monthly_hours * 100])
+            for project, work_hours_sum in self.work_hours_per_project.items()
+        }
+
+        self.assertEqual(expected_result, projects_work_percentage)


I think that in these assertion is too much logic. These tests are too complicated and hard to maintain. These tests should be much simpler. You should in setUp number of hours which assume should be in final report and then check them. We can talk about it if it is not clear for you

The logic has been simplified by preparing the expected result on the test setup level.

kbeker · 2020-06-09T06:01:31Z

utils/mixins.py

-            user = self.object
-        else:
-            user = self.request.user
+        report_set = self.object.report_set.all() if self.model == CustomUser else self.get_queryset()


Instead of checking self.model == CustomUser you can check it this way -> isinstance(self.model, CustomUser)

That's not an instance: it's the class.
Changed == operator to is.

kbeker · 2020-06-09T06:12:27Z

utils/mixins.py

+        Returns dict where keys are Project names and values are lists containing total sum of work hours
+        and percentage amount of work by this user.
+        """
+        work_hours_per_project = report_set.values("project__name").annotate(total_hours=Sum("work_hours"))


Please create from this method. Method name would say what exactly is done with this query.

kbeker · 2020-06-09T06:12:33Z

utils/mixins.py

+        and percentage amount of work by this user.
+        """
+        work_hours_per_project = report_set.values("project__name").annotate(total_hours=Sum("work_hours"))
+        work_hours_sum = work_hours_per_project.aggregate(Sum("total_hours"))["total_hours__sum"]


Same here :)

kbeker · 2020-06-09T06:17:31Z

utils/mixins.py

+                [annotation["total_hours"], (annotation["total_hours"] / work_hours_sum) * 100]
+                if work_hours_sum.total_seconds() > 0
+                else 0


Please extract this to separate private method and name it well

Which section exactly? It's a short code and a fairly simple logic of what is already a private method, which name already explains what it does: get dict of total work hours per project statistics. If name isn't descriptive enough I could change it, but it's already a mouthful. I can also add a comment explaining what happens here.

kbeker · 2020-06-22T06:41:07Z

employees/tests/test_views.py

+    def _generate_user_reports_for_current_and_previous_month_with_uneven_total_hours(self):
+        reports_per_project, remaining_hours = self._strip_hours_between_projects()
+
+        months_and_reported_days = self._get_number_of_reported_days_for_each_month(reports_per_project)
+
+        for start_date, total_reported_days in months_and_reported_days.items():
+            total_reported_days = reports_per_project
+            report_date = start_date
+            for _ in range(total_reported_days):
+                work_hours = timezone.timedelta(hours=self.hours_per_report)
+                for project in self.projects:
+                    ReportFactory(project=project, author=self.user, work_hours=work_hours, date=report_date)
+                report_date += relativedelta(days=1)
+            if remaining_hours != 0:
+                ReportFactory(
+                    project=self.projects[0],
+                    author=self.user,
+                    work_hours=timezone.timedelta(hours=remaining_hours),
+                    date=report_date,
+                )
+
+    def _get_number_of_reported_days_for_each_month(self, reports_per_project, difference_ratio=2):
+        current_month_start_date = self.current_time.replace(day=1)
+        previous_month_start_date = current_month_start_date + relativedelta(months=-1)
+        return {
+            previous_month_start_date: reports_per_project // difference_ratio,
+            current_month_start_date: reports_per_project,
+        }
+
+    def _strip_hours_between_projects(self):
+        hours_per_project = self.total_hours // len(self.projects)
+        remaining_hours = self.total_hours % len(self.projects)
+        reports_per_project = hours_per_project // self.hours_per_report
+        remaining_hours += hours_per_project % self.hours_per_report
+        return reports_per_project, remaining_hours
+
+    def _get_total_hours_per_project_and_percentage_from_month(self):
+        reports_per_project, remaining_hours = self._strip_hours_between_projects()
+        first_project_total_hours = timezone.timedelta(
+            hours=reports_per_project * self.hours_per_report + remaining_hours
+        )
+        other_projects_total_hours = timezone.timedelta(hours=reports_per_project * self.hours_per_report)
+        hours_and_percentage_per_project = {}
+        total_hours_timedelta = timezone.timedelta(hours=self.total_hours)
+        for project in self.projects:
+            if project is self.projects[0]:
+                hours_and_percentage_per_project.update(
+                    {project.name: (first_project_total_hours, first_project_total_hours / total_hours_timedelta * 100)}
+                )
+            else:
+                hours_and_percentage_per_project.update(
+                    {
+                        project.name: (
+                            other_projects_total_hours,
+                            other_projects_total_hours / total_hours_timedelta * 100,
+                        )
+                    }
+                )
+        return dict(sorted(hours_and_percentage_per_project.items()))


This looks bad as a test suite. We need to get rid of as quickly as we can.

kbeker · 2020-06-22T06:44:31Z

employees/tests/test_views.py

+        self.assertEqual(self.expected_work_hours_stats, projects_work_percentage)
+
+    def _generate_user_reports_for_current_and_previous_month_with_uneven_total_hours(self):
+        reports_per_project, remaining_hours = self._strip_hours_between_projects()


When you're returning tuple please make in openly. You can avoid any unexpected errors.

kbeker · 2020-06-22T06:46:01Z

employees/tests/test_views.py

+        remaining_hours = self.total_hours % len(self.projects)
+        reports_per_project = hours_per_project // self.hours_per_report
+        remaining_hours += hours_per_project % self.hours_per_report
+        return reports_per_project, remaining_hours


Same about returning from method. If you returning tuple please do it openly

kbeker

I'm gonna merge this. Please see comments which I left below

maciejSamerdak added bug Something isn't working priority high Tasks with high priority labels May 31, 2020

maciejSamerdak requested a review from kbeker May 31, 2020 19:32

maciejSamerdak self-assigned this May 31, 2020

maciejSamerdak force-pushed the bugfix-incorrect-hours-stats branch from 91aa01f to 9b48df1 Compare May 31, 2020 19:46

kbeker suggested changes Jun 9, 2020

View reviewed changes

maciejSamerdak force-pushed the bugfix-incorrect-hours-stats branch from 39d4834 to ce53b84 Compare June 21, 2020 16:46

kbeker reviewed Jun 22, 2020

View reviewed changes

maciejSamerdak added 3 commits June 22, 2020 09:08

Refactor backend for work hours summary functionality

fd6e4da

Update affected unit tests

fe1424b

Add additional unit tests for views

c461cb7

kbeker force-pushed the bugfix-incorrect-hours-stats branch from ce53b84 to c461cb7 Compare June 22, 2020 07:09

kbeker approved these changes Jun 22, 2020

View reviewed changes

kbeker merged commit a40131b into master Jun 22, 2020

kbeker added this to the v0.12.1 milestone Jun 22, 2020

kbeker deleted the bugfix-incorrect-hours-stats branch June 22, 2020 07:25

Conversation

maciejSamerdak commented May 31, 2020

Uh oh!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

kbeker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants