Won't if not issue.assignee: do it?

This is purely personal preference, but would old_assignee = issue.assignee if issue.assignee else "" do what you intend to do here?

@cep - applied your recommendations

I'm just starting the review but this else part is troubling me

This would fail is old_assignee is None or ""

Hm, ok, but then I'd rather is make old_assignee be the .user (ie a string not an object) directly

I'll have to play around with this a little more than I can do it right now as in I am not entirely fan of returning more than one value in a function. It may end up being the best solution, but I'd like to see and experiment if we can do something else.

I'll have to play around with this a little more than I can do it right now as in I am not entirely fan of returning more than one value in a function. It may end up being the best solution, but I'd like to see and experiment if we can do something else.

Yeah I don't like returning two "values" either. Perhaps I can create a very simple "comment class" object that can be passed to each "edit" function for building the final/cumulative notification comment.

This is what cep wanted me to do.

I think I meant else "". Wouldn't it do what you want to do, or is there a special reason to have '\"\"'?

Revised, rebased, and ready for review...

Revised, rebased, and ready for review...

• When another user is already assigned to an issue, clicking the Take button besides the assignee name gives something like Assignee changed from User: 30 - name blah to foo, while it ought to be Assignee changed from blah to foo.

• When a assignee is first added, the comment reads Assignee changed from "" to foo. Is it possible to make it something like Assignee added assignee: foo? And on eventual changes, it can be the ususal Assignee changed from foo to blah.

• Sometimes, I randomly (haven't found the cause yet) get the following error when editing the Blocked or Depends on fields, and on other edits as well, once this error pops up.

  You cannot close a ticket that has ticket depending that are still open.

  I will investigate this further.

Arf, I wanted to push to my repo and instead I pushed to your repo @mreynolds sorry about that.

Anyway, this is just a proposal, it behaves quite differently and there are a couple of unit-tests that are still broken. Unfortunately I have to go right now so I wanted to push these changes (I meant to point you to my branch rather than updating yours) for early feedback and comment :)

Feel free to comment, agree or disagree with the change :)

Things are broken it seems. Every time I try and update an issue's metadata I get:

(raised as a result of Query-invoked autoflush; consider using a session.no_autoflush block if this flush is occurring prematurely) (sqlite3.IntegrityError) NOT NULL constraint failed: issue_values.value [SQL: u'INSERT INTO issue_values (key_id, issue_uid, value) VALUES (?, ?, ?)'] [parameters: (8, u'bb71768ddebf4340b97368e1e7f59b90', None)]

This is happening from doing the first query in set_custom_key_value()

Okay I had to blow my old project away and start over. Now its "working".

So I do have some issues with what you changed. One, you removed "who" made the change, and two, you removed what the old value was. Why did you strip this useful/important information?

I'm sorry, but I strongly feel you need to bring this information back to the notification comment.

And custom fields show that they are being reset with every update, although the custom fields are not being updated.

So I do have some issues with what you changed. One, you removed "who" made the change, and two, you removed what the old value was. Why did you strip this useful/important information?

The who part we could bring back, the old info I figured it was written earlier in the page and if it isn't it means it had no values before.

That being said, what I pushed is a proposal, definitively up for comments and adjustments, I was more interested to know if people like this approach I took or if the one you had seemed better to more people? :)

So I do have some issues with what you changed. One, you removed "who" made the change, and two, you removed what the old value was. Why did you strip this useful/important information?

The who part we could bring back, the old info I figured it was written earlier in the page and if it isn't it means it had no values before.

The issue with this is when you have an issue with many many comments (which is typical for my team), it then becomes tedious trying to find or not find the old value. I see no harm with including this information in the notification comment. To remove/exclude this information is a regression in my mind.

That being said, what I pushed is a proposal, definitively up for comments and adjustments, I was more interested to know if people like this approach I took or if the one you had seemed better to more people? :)

I would like other's opinions as well ;) I have a screen shot of my original proposal at:

https://pagure.io/pagure/issue/raw/files/a89394fc03ea7a747073bf9c511d71b8deafa9d4e51bae32529bbcc21557fd55-Screenshot_from_2017-01-24_15-59-01.png

I see no harm with including this information in the notification comment. To remove/exclude this information is a regression in my mind.

Ok let's do it then :)

Ok I've added a couple of commits which should address some of your concerns.

Tests are still not passing and I bet not everything is covered but could you give it a quick spin and let me know if this is better or not?

Tests should be passing now :)

Looks good to me, ack. Just rebased

Alright, let's merge then :)