Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add history change reason to allow reasoning for the changes #152

Merged
merged 1 commit into from
Jun 2, 2017

Conversation

hmit
Copy link
Contributor

@hmit hmit commented Feb 2, 2015

use changeReason to set the reason. Could be None

@macro1
Copy link
Collaborator

macro1 commented Feb 8, 2015

I'm not completely sold on this. I need to think about it a bit more, but I would rather see some kind of 'save with additional data' function that could pass things like history user, date, and reason into the history entry. I don't really like the way we handle the other special fields either.

I really like the idea of tracking a reason with the model change though.

@hmit
Copy link
Contributor Author

hmit commented Feb 9, 2015

The get_extra_fields(), and create_historical_record() serve kinda that function, no? I can take a dig at the save_additional_data and something like this could work:

def additional_fields():
    extra_columns = [{
        'field_name': "history_date",
         'type': models.DateTimeField(),
         'attr_name': "_history_date",
         'default_value': lambda x...,
    }]

...etc. What do you think?

@macro1
Copy link
Collaborator

macro1 commented Feb 9, 2015

No, the way you've added the new attribute is fine for the way we have it set up right now.

I was thinking it might be nicer to have a function that could be called to save the original model, and allow additional information to be passed along to the historical instance, rather than blindly using a signal the way we are now.

Django Activity Stream does something like this with the way Actions are generated. There is a function that can be called directly to generate the action, or you can hook the function into a signal, capturing all saves with a less specific description.

@ghost
Copy link

ghost commented Sep 11, 2016

+1

@joaojunior
Copy link
Contributor

@hmit Do you continue this? If no, I will continue and make what @macro1 said.

@hmit
Copy link
Contributor Author

hmit commented May 13, 2017

Hey @joaojunior, thanks for reminding me about this.
This is a fairly old code so let me catch up with it.
But please don't feel blocked in any way, and feel free to take it up if I don't get to it by Monday?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants