Skip to content

Add comment to work item together with information about the sender #19

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hendrikmaarand
Copy link
Contributor

This introduces two new special values (##MessageLastReply and ##MessageLastReplyWithSender) and a new configuration option DefaultFieldValuesOnUpdate. DefaultFieldValuesOnUpdate is same as DefaultFieldValues except that its default values are used when an update is applied to a work item. ##MessageLastReplyWithSender allows to set the default value for field History similarly as is currently done for Description with ##MessageBodyWithSender.

This change also unifies the code a bit as we do not need to have special cases for History field.

It should also allow to remove the configuration option OverrideChangedBy and replace it with ##Sender as default value for 'Changed By' on update.

This introduces two new special values (##MessageLastReply and ##MessageLastReplyWithSender) and a new configuration option DefaultFieldValuesOnUpdate. DefaultFieldValuesOnUpdate is same as DefaultFieldValues except that its default values are used when an update is applied to a work item. ##MessageLastReplyWithSender allows to set the default value for field History similarly as is currently done for Description with ##MessageBody. This change also unifies the code a bit as we do not need to have special cases for History field. It should also allow to remove the configuration option OverrideChangedBy and replace it with ##Sender as default value for 'Changed By'.
@hershi
Copy link
Collaborator

hershi commented Dec 19, 2015

First off - sorry for the very long delay in reviewing it.

Overall, the change looks good - thanks for taking the time to make it. The one concern I have is that it would break any existing setup out of the box, and would require an update to all the configs to make sure that update messages are indeed properly adding the last message to the history.
My suggestion is to change the default behavior such when the relevant config section is missing (i.e. when DefaultFieldValuesOnUpdate is an empty list or null), it works the same way as the legacy code.

The rest of it looks good, and I like the further simplification of the code (no special handling of History field) - it definitely yields a nicer abstraction and more flexible behavior

@hendrikmaarand
Copy link
Contributor Author

Your concern seems valid. Our Mail2Bug set up had a base template from which we generated all the instance configurations. That is why the issue probably did not occur to me as this kind of configuration change only required a simple change to the base template.

Regarding the proposed changes, the problem for me is that I do not have the development/testing environment for this anymore and I am not too inclined to set something up. Maybe this change can then just wait for some bigger release.

@hershi
Copy link
Collaborator

hershi commented Dec 23, 2015

No worries Hendrik. I'll try to take a look once I have some time and see if I can add my suggestion to the code you created and then commit

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.

2 participants