search

samanthasgroup / django-webapps

Backend for the database, implemented in Django
Other
3 stars 0 forks source link

[BACKLOG] Check tests of status setters #319

Open lemontree210 opened 1 year ago

lemontree210 commented 1 year ago

See https://github.com/samanthasgroup/django-webapps/pull/313#pullrequestreview-1688294460 - tests can pass even when status was erroneously used instead of log event type in comparison, if their string value is the same.

Auch-Auch commented 1 year ago

I would suggest postponing it for a bit, simply adding is will not resolve the problem, let's just be careful with them and maybe transform the issue to something like: "Double check status setters"

lemontree210 commented 1 year ago

I would suggest postponing it for a bit, simply adding is will not resolve the problem, let's just be careful with them and maybe transform the issue to something like: "Double check status setters"

Ok. Can you briefly describe why it wouldn't resolve the problem and what is your general idea about how to solve it?

Maybe we should just rename all events to make sure they are not called the same as statuses? Obviously, I mean string values, not names of constants.

Auch-Auch commented 12 months ago

In most of the cases, we are still comparing str to choice or vice versa, "is" will work only with choice to choice, so we can either make it unique or keep it as is, In the database, it will be stored correctly. It is kind of difficult to imagine some scenario where it will lead to a critical error anyway. Might confuse though ;)