Hi Corey,
I had started working on this a looooooong time ago (when Bodhi 2 was
still supposed to be written with TG2 ^^') but due to lack of time I had
to stop. [1]
So I'm very happy you picked it up. :)
One thing you've done and that I wanted to do was to use an Enum for the
Karma types. I had made it with a clumsy database table, and it felt
dirty.
Now if I may, I'd like to offer a quick review of your patch.
config.add_view('bodhi.views.view_model',
- accept='text/html',
context='bodhi.resources.BodhiResource',
+ accept='text/html',
renderer='bodhi:templates/model.mak')
Why change the order here?
-from sqlalchemy.orm import scoped_session, sessionmaker, relation,
relationship
+from sqlalchemy.orm import scoped_session, sessionmaker, relation
[... snip ...]
- bugs = relationship('Bug', secondary=update_bug_table,
- backref='updates', lazy=False)
+ bugs = relation('Bug', secondary=update_bug_table,
+ backref='updates', lazy=False)
This is backwards:
"""relationship() is historically known as relation() prior to version
0.6."""
http://www.sqlalchemy.org/docs/orm/relationships.html?highlight=relation#...
So if relationship is the new name, it might mean that relation could
get deprecated in the future. If you want to unify it all, I'd say it
would be better to use relationship everywhere?
Other than that, your implementation seems a bit too simplistic compared
to what QA had asked. Correct me if I'm wrong, but it seems that each
comment can only add one type of karma, whereas QA seemed to want:
- some karma related to the update (which I had called global karma in
my branch) : still works for me, fails for me, passes test case,
introduces new errors,...
- some karma related to the bugs the update is supposed to fix: fixes
bug 1234 as advertised, doesn't fix bug 1234
- one comment could combine one global karma and one karma per bug
linked to the update (see screenshots from my attempt) [1]
All in all, I think you should have a look at what I had done, as I had
spent quite some time to make sure that:
- it was possible to make one comment with different types of karma
- email bodies would show all the karmas as expected
I'm not saying you have to take it as is, it's been done a long time ago
(and I don't think my opinion is authoritative for Bodhi anyway).
However it could certainly give you a few ideas. Perhaps you could even
just port it to use an enum as you have done here and everything would
work great. (I doubt it though, that would mean I actually wrote some
good code :P)
In any case Luke, for the automatic stable and unstable karma handling,
what is needed is a policy from FESCo (or even just some draft
guidelines from QA) to write the rules, as they will obviously be more
complicated than just a numeric threshold. This is most of what I had
written in the TODO paragraph on the page. [1]
I guess it's time to revive those discussions with QA and FESCo?
Again, thanks for taking over this, I kept wanting to go back to it but
haven't gotten the time to in more than a year now.
Cheers,
[1]
http://bochecha.fedorapeople.org/bodhi_karma_revamped/#todo
--
Mathieu