Fix #1310: If a user edits an update after Bodhi comments it can be pushed, it won't comment again #1396
Conversation
| approve_testing.main(['nosetests', 'some_config.ini']) | ||
|
|
||
| bodhi = self.db.query(models.User).filter_by(name=u'bodhi').one() | ||
| cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id) |
There was a problem hiding this comment.
It would be good to assert that there are three comments here.
| update.comment(self.db, u"Removed build", 0, u'bodhi') | ||
| update.status = models.UpdateStatus.testing | ||
| update.date_testing = datetime.now() | ||
| self.assertEqual(update.meets_testing_requirements, False) |
There was a problem hiding this comment.
I don't think we need the above two lines, as they are really asserting that meets_testing_requirements works.
| update.request = None | ||
| update.status = models.UpdateStatus.testing | ||
| update.date_testing = datetime.now() - timedelta(days=14) | ||
| self.assertEqual(update.days_in_testing, 14) |
There was a problem hiding this comment.
I recommend dropping this line too, since it's just testing that the line above it works.
| self.assertEqual(update.days_in_testing, 14) | ||
| text = unicode(config.get('testing_approval_msg') % update.days_in_testing) | ||
| update.comment(self.db, text, author=u'bodhi') | ||
| self.assertEqual(update.meets_testing_requirements, True) |
There was a problem hiding this comment.
I recommend dropping this line since it's testing meets_testing_requirements(). We can rely on tests elsewhere to make sure that method works.
bodhi/server/models.py
Outdated
| # simply exit this loop. | ||
| break | ||
| else: | ||
| tempcomments.append(comment) |
There was a problem hiding this comment.
If you use Python's yield statement in this method, it will become a generator and will not have to load all of the comments into a temporary list. This will make the code a bit more efficient, nearly as efficient as it had been before (though incorrect before ☺). Also, you do not need the else clause because not hitting the break implies that the if statement wasn't matched. Thus, I recommend refactoring the above to something like this:
for comment in reversed(self.comments):
if (comment.user.name == u'bodhi' and
('New build' in comment.text or 'Removed build' in comment.text)):
yield comment
bodhi/server/models.py
Outdated
| @@ -818,6 +808,31 @@ def _composite_karma(self): | |||
|
|
|||
| return positive_karma, negative_karma | |||
|
|
|||
| def get_comments_since_karma_reset(self): | |||
There was a problem hiding this comment.
I recommend naming this comments_since_karma_reset() and using the @property decorator on it. Something like this:
@property
def comments_since_karma_reset(self):
…
This will allow callers to use it like an attribute instead of having to explicitly call it, which is kind of nice. For example, callers can use update.comments_since_karma_reset instead of update.get_comments_since_karma_reset(). A lot of the methods on the update class work like this, so it would fit in nicely.
|
@bowlofeggs I implemented the changes. Using the yield statement is much more efficient. Thank you so much for your helpful feedback! |
bodhi/server/models.py
Outdated
| Return the comments since the most recent karma reset event, which | ||
| occurs whenever a build is added or removed from an Update. | ||
| :return: a list of comments since karma reset | ||
| :rtype: list |
There was a problem hiding this comment.
Technically this is returning a generator now. I'd change this from list to generator, and you can change the line above this to say "an iterable of comments…" or generator if you prefer.
| approve_testing.main(['nosetests', 'some_config.ini']) | ||
| update.comment(self.db, u"Removed build", 0, u'bodhi') | ||
| update.status = models.UpdateStatus.testing | ||
| update.date_testing = datetime.now() - timedelta(days=14) |
There was a problem hiding this comment.
We also don't need this line or the one above it since lines 346 and 348 above already makes these same changes.
|
|
||
| with patch( | ||
| 'bodhi.server.scripts.approve_testing._get_db_session', return_value=self.db): | ||
| approve_testing.main(['nosetests', 'some_config.ini']) |
There was a problem hiding this comment.
You probably don't need this line, since line 349 already adds the comment. So you could drop this line, or you could drop lines 348 and 349.
crungehottman
force-pushed
the
iss1310
branch
4 times, most recently
from
afa6d94
to
ce0de5a
Compare
|
Changes made! Thanks again. |
There was a problem hiding this comment.
@crungehottman This looks great! I'm sorry I didn't notice this before, but I have oooone more change to ask for. Can you change your commit message to be more descriptive, and move the bug reference number in to the commit message body? Something like this would do:
Bodhi's testing approval comment now respects the karma reset event
<feel free to expand if you want by putting a longer description here, but not required>
fixes #1310
Feel free to season to taste, but the general idea is to give a short but descriptive summary of the change, an optional more descriptive body, and a last line with the fixes #1310 so that GitHub will autoclose the associated issue. That fixes is also handy when people want to know what issue this was associated with.
|
I think it should be okay now! Thanks for the tip |
should fix issue #1310