Feature project should have customisable task activity list by Szymiks · Pull Request #460 · Code-Poets/sheetstorm

Szymiks · 2019-07-29T10:20:22Z

Resolves: #204

This PR will be change because @Karrp want this feature in differemt way.
Related:#469

In this PR I just created few things:
Should be done

create fields in task_activite_models - projects - m2m and is_default
refactor singals to signals.py in manager app because I had problems with import TaskActivityType in models in manager app and import Project in models in employee app.
view when Admin and manager could manage project task activities or create new one.

Will be done in next PR

add view for Admin to manage all task activities
add dynamic filter during filling report

Question
I have never done something like this, so when I should make some script or something to set default task activity list for all existing projects?

Szymiks · 2019-07-29T10:35:28Z

@Karrp Could you say what task activities should be default for all projects or maybe talk about it with Adrian ? :)

rwrzesien · 2019-07-29T20:55:02Z

I have never done something like this, so when I should make some script or something to set default task activity list for all existing projects?

For existing projects, in data migration.
For new ones, in function connected to post_save signal to Project model.

employees/models.py

rwrzesien · 2019-07-29T21:02:39Z

managers/apps.py

    name = "managers"
+
+    def ready(self) -> None:
+        import managers.signals  # noqa, flake8 F401 issue  # pylint: disable=unused-import


Why do you need to import it this way?

😱 ⛔ 🚫 🙀

@rwrzesien , @kbeker I couldn't import TaskActivityType in manager app in models so I decided to refactor whole handlers to file signals.py. On this page, It is written that we have to do this because if I wouldn't my handlers will not catch signals.
https://simpleisbetterthancomplex.com/tutorial/2016/07/28/how-to-create-django-signals.html

What was the exact problem with imports? Maybe it could be avoided by just importing inside the method/function, which would be easier than moving a lot of code around.

I hade this exception

Traceback (most recent call last): File "manage.py", line 19, in <module> execute_from_command_line(sys.argv) File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line utility.execute() File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/site-packages/django/core/management/__init__.py", line 357, in execute django.setup() File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/site-packages/django/__init__.py", line 24, in setup apps.populate(settings.INSTALLED_APPS) File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/site-packages/django/apps/registry.py", line 114, in populate app_config.import_models() File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/site-packages/django/apps/config.py", line 211, in import_models self.models_module = import_module(models_module_name) File "/home/szymi/virtualenv/sheetstorm-SgY9UmI-/lib/python3.7/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 1006, in _gcd_import File "<frozen importlib._bootstrap>", line 983, in _find_and_load File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 677, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 728, in exec_module File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed File "/home/szymi/Dokumenty/GitRepos/sheetstorm/sheetstorm/managers/models.py", line 9, in <module> from employees.models import TaskActivityType File "/home/szymi/Dokumenty/GitRepos/sheetstorm/sheetstorm/employees/models.py", line 14, in <module> from managers.models import Project ImportError: cannot import name 'Project' from 'managers.models' (/home/szymi/Dokumenty/GitRepos/sheetstorm/sheetstorm/managers/models.py)

I think it happen because We imported Project in employees app and in managers app models we want to import TaskActivityType which is needed to work with handlers.

@rwrzesien When I make import locally it works fine but
sheriff pylint says employees/templatetags/data_structure_element_selectors.py:1:0: R0401: Cyclic import (employees.models -> managers.models) (cyclic-import)

I see that you have resolved this by making import directly in function, so it works now right?

@rwrzesien , Yes it works but pylint gives one warning about cycling import

managers/signals.py

managers/templates/managers/project_task_activities.html

managers/views.py

rwrzesien · 2019-07-29T21:09:47Z

managers/views.py

+        return super().get(request, args, kwargs)
+
+    def post(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
+        self.object = self.get_object()  # pylint: disable=attribute-defined-outside-init


Is this needed?

Yes, FormView doesn't have self.object so I inherit from SingleObjectMixin and use all methods from there to work with object and queryset. What is more it give protection to this view only for manager of current project, because without SingleObjectMixin this mixin IsManagerOfCurrentProjectMixin doesn't work.

What is more when I wanted to use self.object during post I have to init this variable because then in form_valid I don't have it .

What if you combine FormView and DetailView instead SingleObjectMixin?

Sure I will do this

So is this still needed after that?

Yes it is, because I am using self.object here -> _add_task_activities_to_relation

managers/views.py

rwrzesien

Not bad in general, but some stuff has to be improved.
Also, it would be good to compare this pull request with actual requirements to see if it fits them, but I couldn't find them anywhere described.

Karrp · 2019-07-30T07:53:11Z

@Karrp Could you say what task activities should be default for all projects or maybe talk about it with Adrian ? :)

@Szymiks
If you create new project task activities should be None as default. Consequently when creating report employee should be able to chose every possible activity from list.

kbeker

There are some things to polish. But in overall good job :)

employees/models.py

kbeker · 2019-07-30T07:44:50Z

managers/apps.py

    name = "managers"
+
+    def ready(self) -> None:
+        import managers.signals  # noqa, flake8 F401 issue  # pylint: disable=unused-import


😱 ⛔ 🚫 🙀

managers/static/managers/style.css

managers/tests/test_unit_project_signals.py

managers/tests/test_views.py

managers/views.py

Karrp · 2019-07-30T08:36:11Z

Hmm it is quite edge case but we still need to handle this. What to do with reports which have activity which can be deleted by manager from list of project_activities?

We can do 2 things i think.

Not delete just inactive (not to choose in new reports -> edition? ) - more changes, need discussion
Delete and change to Other - easier but changes history

@kbeker what you think?

dybi

I see a lot of work, great deal of which is in a good direction. However, some parts need solid polishing.

managers/tests/test_views.py

managers/views.py

managers/tests/test_views.py

managers/signals.py

managers/templates/managers/project_task_activities.html

dybi

Some clarifications are still needed.

managers/signals.py

managers/tests/test_views.py

dybi

managers/models.py

rwrzesien · 2019-08-02T23:17:25Z

managers/models.py

-            continue
+    CustomUser.objects.filter(pk__in=pk_set).exclude(
+        Q(user_type=CustomUser.UserType.ADMIN.name) | ~Q(manager_projects=None)
+    ).update(user_type=CustomUser.UserType.EMPLOYEE.name)


Nice solution 👍

rwrzesien · 2019-08-02T23:23:13Z

@Szymiks Almost there, I think the one important thing left is passing task_activity_id in url and make proper html in the table.

managers/tests/test_views.py

kbeker

I think that generally everything is OK here :) before merge I will test it also manually, but now we waiting for all approves :)

Except refactor, I also added post_save receiver to add default tasks for new project

Szymiks added feature New feature priority low Tasks with low priority labels Jul 29, 2019

Szymiks requested review from Karrp, MartynaAnnaGottschling, dybi, kbeker, maciejSamerdak and rwrzesien July 29, 2019 10:20

Szymiks self-assigned this Jul 29, 2019