Add description argument to fsm_log_description decorator

[lorenzomorandini]

The aim of this pull request is to give the ability to add a default description to the fsm_log_description.
The priority would be:

1. "description" passed as argument when a transition is called
2. "description" set inside the transition if allow_inline is True
3. default "description" set as default

Example:

@fsm_log_description(description='default')
def execute(self, description=None):
...

I find it quite useful since nearly always I have a fixed description to set when running a transition.

[ticosax]

Thanks, I feel the need to challenge your motivation before moving forward with this.

I find it quite useful since nearly always I have a fixed description to set when running a transition.

If it's a fixed description, why to you need to store it in your database?

[lorenzomorandini]

Well it's fixed but it can change with a new software release, I want it stored in the database so I can have the history of it and even show it to users.

Django-fsm-log already does everything, I just personally think it's more intuitive to write it as I am doing. One of the nice things about django-fsm is that by looking at the transition method a developer can easily see all the conditions, the from/to states and so on. Having the log description in another place (typically where you call the transition method) is less intuitive to me. This pull request just adds a new way to set it, while still giving priority to the already present ways.

[ticosax]

Well it's fixed but it can change with a new software release, I want it stored in the database so I can have the history of it and even show it to users.

It still feel to me, that you intent to store in the database a value that could be, instead, computed. If I understand your use case correctly, you could programmatically provide this description without having to store it in the database.
Storing in the database, values that could be computed, instead, fell in the category of questionable practice of software development (according to my own believes, highly subjective).
This why, unless presented with a use case that can not be solved otherwise, I'm leaning towards declining adding this new functionality.
I can be convinced otherwise, given the right arguments.

[lorenzomorandini]

It doesn't always be computed. For an example, today I could have a transaction that has the description "Goods packaged and shipped". It's static.

In a month, I change it with "Good packaged, shipped, waiting for pick up".

I still want the old transitions to have the old description, only the new ones will have the new description. If I want to change the previous ones too I'll use a database migration.

[ticosax]

I still want the old transitions to have the old description, only the new ones will have the new description. If I want to change the previous ones too I'll use a database migration.

Ultimately, this business logic can be supported programmatically, since you (as a developer) are in control of when the description changes. There is no user input here.

[lorenzomorandini]

I understand but still don't see it as bad practice to store a fixed string in the database. A query to fetch the logs is surely faster than calculating every time the same fixed string in python.

[ticosax]

I really don't want to add this functionality as it motivated by a use case that violates one of my stongest belief that data should come first, before code, but I don't own this django-fsm-log, therefore I don't want to impose my point of view unilaterally either.

What the rest of the community think about it ?
I'm willing to hear other opinions before moving forward with this.

[RokuDagy]

Would love to see this implemented.

[Robert-Lebedeu]

I am with Lorenzo on this one. I get his point.
I've used this package multiple times and I think that it would be really nice to add a default description to the decorator.
I don't see it as bad practice, storing a "fixed" string considering that it may change on new SW releases seems ok to me.

As Lorenzo pointed out, you already can achieve that by simply passing the same string over and over, basically every time you call the transaction method.
I simply find it way cleaner to allow a default description when using decorator.

[ticosax]

alright, thanks everyone for your feedback.
In in order to move forward with this, @lorenzomorandini would you like to add some tests, add a entry in the changelog and document it ?

[lorenzomorandini]

@ticosax sure, will work on it

[lorenzomorandini]

@ticosax let me know if it's starting to look ok, thank you

[lorenzomorandini]

Hi, just to understand, are we waiting for #144 ? Or do I need to fix something here?

[ticosax]

Hi, just to understand, are we waiting for #144 ? Or do I need to fix something here?

More or less, yes. The tests are not passing on master, we need to fix this first, so you can rebase your branch once master it's fixed.

[samueljoli]

@lorenzomorandini @ticosax, Initially, I planned on using fsm_log_description to add additional metadata about a transition.

For example:
During a transition an exception was thrown, and therefore I'd like to transition the subject to an "error state". The error state is generic like validation_failed. For the purpose of debugging I'd like to store specifics of what failed validation in the log description.

Since reading this thread I think that this might be a misuse of this decorator. I'm making this assumption since there isn't any official documentation on this decorator at the moment.

If this is indeed a misuse, would you have any suggestions on how I can achieve this functionality.

[lorenzomorandini]

@samueljoli I guess it can be used like that too, with the runtime "description.set(...)".

[codecov]

Codecov Report

Merging #143 (8adc148) into master (d3360be) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   95.05%   95.13%   +0.07%     
==========================================
  Files          25       25              
  Lines         506      534      +28     
==========================================
+ Hits          481      508      +27     
- Misses         25       26       +1     

Impacted Files	Coverage Δ
django_fsm_log/decorators.py	100.00% <100.00%> (ø)
tests/models.py	98.07% <100.00%> (+2.95%)	⬆️
tests/test_model.py	100.00% <100.00%> (ø)
django_fsm_log/helpers.py	89.47% <0.00%> (-10.53%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ticosax]

Nice