Add tags management for cases #950
Conversation
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Add missing migrations [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Fix tests
There was a problem hiding this comment.
Good job generally :-).
Although I'm wondering why so much code has to be involved to implement quite simple and generic feature. It's probably due to a single layer architecture, Django template language limitations, tranlations and some previous architectural decisions. The UI is also quite unintuitive and unfriendly comparing to modern apps. So the more time I spend in this project, the more I start to dubt if it's worth to put so much effort in developing it in it's current shape, or would it be better to think about rewriting most of it in backend/frontend architecture with new UI design.
But it's little bit off topic, so sorry :-).
| @@ -58,6 +58,7 @@ | |||
| "feder.institutions", | |||
| "feder.monitorings", | |||
| "feder.cases", | |||
| "feder.cases_tags", | |||
There was a problem hiding this comment.
Do we really need so much granulation? To create a new app with single model instead adding it to existing cases app, as name suggests?
There was a problem hiding this comment.
I don't see a problem with such a large application granularity. Originally I wrote this in one application, but later it causes a problem in creating url eg. reverse("cases: list") (list of what? tags or case?). So I started to rewrite it as a sub-application. In the end, I wrote this as a separate app and I like it because CRUD around tags is contained together, CRUD around a case is contained together, and so on. You can easily navigate through the project in this way and views.py and models.py is not too long without in-consistent spliting.
We could rewrite all urls to make routing look like <category>:<model>-<action>, but now we have de facto <model>: <action>, which pushes us into separate applications for a separate model with our own life cycle.
I can see that for Django Rest Framework it would be mostly one ViewSets, but the current CBV based on the Django vanilla is adopted.
There was a problem hiding this comment.
Ok, it looks like it's enough of separate code to make dedicated app for it. Although making it as cases app part would also make sense, because logically they are tightly coupled, so that's where my question came from.
| @@ -31,6 +31,9 @@ def area(self, jst): | |||
| case__institution__jst__lft__range=(jst.lft, jst.rght), | |||
| ) | |||
|
|
|||
| def for_user(self, user): | |||
There was a problem hiding this comment.
What's this for? I believe there is some reason for this method to exist, but the code does not suggest anything and there is no description, so I got bit confused here.
There was a problem hiding this comment.
Some time ago, Bartosz Wilk reported that he would like to have private monitoring (Unfortunatly, I can not find issue for that). I can see that private monitoring is a big thing that requires going through the entire application. I'm afraid that when we get down to it, we'll miss it and cause leak, so I introduced a method to go in that direction in small steps. In the future, we will introduce monitoring filtering based on some basic right, eg "view_monitoring".
That approach for permission via QuerySet was adopted in Watchdog Python project earlier ( https://github.com/search?l=Python&q=org%3Awatchdogpolska+for_user&type=Code ).
There was a problem hiding this comment.
Ok, I understand now. But a comment in the code would also be helpful there.
|
And minor thing, field in tag filter form is getting translated as: So from my side that's all. When you merge it, I'll start to use it in my branch for monitoring report. |
No description provided.