#172 Show discussion vote counts in web UI and in IRC format
Merged a month ago by lbrabec. Opened 2 months ago by lbrabec.

@@ -0,0 +1,27 @@ 

+ """Added votes to bug

+ 

+ Revision ID: 72031671860e

+ Revises: 4eac64cdecd3

+ Create Date: 2021-02-04 11:14:11.208780

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = '72031671860e'

+ down_revision = '4eac64cdecd3'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ 

+ def upgrade():

+     # ### commands auto generated by Alembic - please adjust! ###

+     op.add_column('bug', sa.Column('votes', sa.Text(),

+                                        server_default=sa.sql.text('"{}"'), nullable=False))

+     # ### end Alembic commands ###

+ 

+ 

+ def downgrade():

+     # ### commands auto generated by Alembic - please adjust! ###

+     op.drop_column('bug', 'votes')

+     # ### end Alembic commands ###

@@ -25,9 +25,12 @@ 

  from sqlalchemy import func, desc, or_, and_

  import bugzilla

  from flask_fas_openid import fas_login_required

+ import json

+ import itertools

  

  from blockerbugs import app, db, __version__

  from blockerbugs.util.bz_interface import BlockerProposal, BZInterfaceError

+ from blockerbugs.util import pagure_bot

  from blockerbugs.models.bug import Bug

  from blockerbugs.models.milestone import Milestone

  from blockerbugs.models.update import Update
@@ -178,6 +181,88 @@ 

      }

  

  

+ def bugz_to_votes(bugz):

+     """Returns votes for each bug in bugz in format:

+     {

+         bugid: {

+             tracker_name: {

+                 "-1": ["nick1", "nick2"],

+                 "0": [],

+                 "+1": ["nick3"],

+             }

+         }

+     }

+     """

+     # ignore milestone key and flatten

+     all_bugz = itertools.chain(*bugz.values())

+     votes = {}

+     for bug in all_bugz:

+         votes[bug.bugid] = json.loads(bug.votes)

+     return votes

+ 

+ 

+ def irc_voting_info(bugz):

+     """Returns voting summary in IRC format for each bug in bugz:

+     {

+         bugid: "#info Ticket vote: ...\n#info Ticket vote: ..."

+     }

+     """

+     voting_info = {}

+     all_votes = bugz_to_votes(bugz)

+     for bugid, votes in all_votes.items():

+         info = ""

+         for tracker, vote in votes.items():

+             # if no one voted +1 or -1, don't show the info at all

+             if not (vote['-1'] or vote['+1']):

+                 continue

+             summary = f"(+{len(vote['+1'])},{len(vote['0'])},-{len(vote['-1'])})"

+             pros = [f"+{person_vote}" for person_vote in vote['+1']]

+             neutrals = [f"{person_vote}" for person_vote in vote['0']]

+             cons = [f"-{person_vote}" for person_vote in vote['-1']]

+             people = ", ".join(pros + neutrals + cons)

+             info += f"#info Ticket vote: {pagure_bot.NICER[tracker]} {summary} ({people})\n"

+         voting_info[bugid] = info.strip()

+ 

+     return voting_info

+ 

+ 

+ def vote_count_tuple(votes, tracker_name):

+     """Returns voting tuple for given tracker_name in format:

+     (6, 0, 3) or None

+     """

+     vote = votes.get(tracker_name)

+     if not vote:

+         return None

+ 

+     return (len(vote['+1']), len(vote['0']), len(vote['-1']))

+ 

+ 

+ def web_voting_info(bugz, milestone):

+     """Returns voting tuple for each bug and each section in web UI,

+     a dict in dict structure in format:

+     {

+         bugid: {

+             'Prioritized Bugs': None,

+             'Proposed Blockers': (6, 0, 3),

+             ...

+         }

+     }

+     """

+     voting_info = {}

+     all_votes = bugz_to_votes(bugz)

+     for bugid, votes in all_votes.items():

+         voting_info[bugid] = {

+             'Proposed Blockers': vote_count_tuple(votes, f'{milestone}blocker'),

+             'Accepted Blockers': vote_count_tuple(votes, f'{milestone}blocker'),

+             'Proposed Freeze Exceptions': vote_count_tuple(votes, f'{milestone}freezeexception'),

+             'Accepted Freeze Exceptions': vote_count_tuple(votes, f'{milestone}freezeexception'),

+             'Accepted 0-day Blockers': vote_count_tuple(votes, '0day'),

+             'Accepted Previous Release Blockers': vote_count_tuple(votes, 'previousrelease'),

+             'Prioritized Bugs': None  # no discussion for prioritized bugs

+         }

+     return voting_info

+ 

+ 

  @main.route('/')

  def index():

      if app.debug:
@@ -205,13 +290,15 @@ 

      bugz = get_milestone_bugs(milestone)

      recent_bugs = get_recent_modifications(milestone.id)

      whiteboard_change = get_recent_whiteboard_change(milestone.id)

+     vote_info = web_voting_info(bugz, milestone.version)

      return render_template('blocker_list.html',

                             buglists=bugz,

                             recent=recent_bugs,

                             wb_change=whiteboard_change,

                             info=release_info,

                             title="Fedora %s %s Blocker Bugs" % (

-                            release_info['number'], release_info['phase']))

+                            release_info['number'], release_info['phase']),

+                            vote_info=vote_info)

  

  

  @main.route('/bug/<int:bugid>/updates')
@@ -236,8 +323,9 @@ 

          abort(404)

      bugz = get_milestone_bugs(milestone)

      milestone_info = get_milestone_info(milestone)

- 

-     response = make_response(render_template('irc_format.txt', buglists=bugz, info=milestone_info))

+     vote_info = irc_voting_info(bugz)

+     response = make_response(render_template('irc_format.txt', buglists=bugz, info=milestone_info,

+                                              vote_info=vote_info))

      response.mimetype = 'text/plain'

      return response

  
@@ -258,6 +346,7 @@ 

                             updates=updates,

                             title="Fedora %s %s Blocker Bug Updates" % (milestone_info['number'], milestone_info['phase']))

  

+ 

  @main.route('/milestone/<int:num>/<release_name>/requests')

  def display_release_requests(num, release_name):

      release = Release.query.filter_by(number=num).first()
@@ -274,6 +363,7 @@ 

      response.mimetype = 'text/plain'

      return response

  

+ 

  @main.route('/milestone/<int:num>/<milestone_name>/need_testing')

  def display_updates_need_testing(num, milestone_name):

      release = Release.query.filter_by(number=num).first()

file modified
+12
@@ -54,6 +54,7 @@ 

                                  backref=db.backref('bugs', lazy='dynamic'))

                                  #backref='bugs')

      discussion_link = db.Column(db.String, unique=False)

+     votes = db.Column(db.Text)

  

      def __init__(self, bugid, url, summary, status, component, milestone,

                   active, needinfo, needinfo_requestee,
@@ -79,6 +80,17 @@ 

          self.prioritized = False

          self.last_whiteboard_change = last_whiteboard_change

          self.last_bug_sync = last_bug_sync

+         # self.votes is JSON representation of dict of all BugVoteTrackers

+         # and corresponding output of BugVoteTracker.enumerate_votes()

+         # with comment ids removed:

+         # {

+         #    tracker_name: {

+         #        "-1": ["nick1", "nick2"],

+         #        "0": [],

+         #        "+1": ["nick3"],

+         #    }

+         # }

+         self.votes = "{}"

  

      def __repr__(self):

          return '<bug %d: %s>' % (self.bugid, self.summary)

@@ -77,7 +77,15 @@ 

                          <td>{{ bug.component }}</td>

                          <td>{{ bug.status }}</td>

                          <td>{{ bug.summary }}</td>

-                         <td><a href='{{ bug.discussion_link }}' target="_blank" rel="noopener noreferrer">{{ 'Discuss' if buglist.startswith('Accepted') else 'Vote!' }}</a></td>

+                         <td class="text-center">

+                             {% if vote_info[bug.bugid][buglist] %}

+                             <span class="{{'text-success' if  vote_info[bug.bugid][buglist][0] > 0 else ''}}">+{{ vote_info[bug.bugid][buglist][0] }}</span>,

+                             <span>{{ vote_info[bug.bugid][buglist][1] }}</span>,

+                             <span class="{{'text-danger' if  vote_info[bug.bugid][buglist][2] > 0 else ''}}">-{{ vote_info[bug.bugid][buglist][2] }}</span>

+                             <br />

+                             {% endif %}

+                             <a href='{{ bug.discussion_link }}' target="_blank" rel="noopener noreferrer">{{ 'Discuss' if buglist.startswith('Accepted') else 'Vote!' }}</a>

+                         </td>

                          <td class="popupification">

                              <a href="{{ url_for('main.display_bug_updates', bugid=bug.bugid) }}"

                                  rel="{{ url_for('main.display_bug_updates', bugid=bug.bugid) }}"

@@ -21,6 +21,8 @@ 

  #link {{ bug.discussion_link }}

  {% endif -%}

  #info {{ buglist | replace("Blockers", "Blocker") }}, {{ bug.component }}, {{ bug.status }}

+ {{ vote_info[bug.bugid] }}

+ 

  

  {% endfor -%} {# end of bug iteration #}

  {% endfor %} {# end of buglist iteration #}

file modified
+45 -14
@@ -3,9 +3,12 @@ 

  import datetime

  import re

  import collections

+ import json

  

- from blockerbugs import app

+ from blockerbugs import app, db

  from blockerbugs.util import pagure_interface

+ from blockerbugs.models.milestone import Milestone

+ from blockerbugs.models.bug import Bug

  

  TRACKER_KEYWORDS = [

      'betablocker',
@@ -16,6 +19,17 @@ 

      'previousrelease'

  ]

  

+ NICER = {

+     'betablocker': 'BetaBlocker',

+     'finalblocker': 'FinalBlocker',

+     'betafreezeexception': 'BetaFreezeException',

+     'finalfreezeexception': 'FinalFreezeException',

+     '0day': '0Day',

+     'previousrelease': 'PreviousRelease',

+     'accepted': 'Accepted',

+     'rejected': 'Rejected'

+ }

+ 

  VOTES = ("+1", "0", "-1")

  TRACKER_RE = r'((beta|final)(blocker|fe|freezeexception)|0day|previousrelease)'

  TRACKER_MATCHER = re.compile(TRACKER_RE)
@@ -302,24 +316,13 @@ 

      :param str tracker_name: e.g. 'BetaBlocker'

      :param tracker: an instance of BugVoteTracker

      '''

-     nicer = {

-         'betablocker': 'BetaBlocker',

-         'finalblocker': 'FinalBlocker',

-         'betafreezeexception': 'BetaFreezeException',

-         'finalfreezeexception': 'FinalFreezeException',

-         '0day': '0Day',

-         'previousrelease': 'PreviousRelease',

-         'accepted': 'Accepted',

-         'rejected': 'Rejected'

-     }

- 

      out = "* "

  

      outcome = ""

      if not tracker.open:

-         outcome = "**%s** " % nicer[tracker.outcome]

+         outcome = "**%s** " % NICER[tracker.outcome]

  

-     out += "%s%s " % (outcome, nicer[tracker_name])

+     out += "%s%s " % (outcome, NICER[tracker_name])

  

      votes = tracker.enumerate_votes()

      pros = len(votes['+1'])
@@ -452,3 +455,31 @@ 

      app.logger.debug('Updating issue summary')

      vote_summary = summary(trackers, non_voting_users, last_comment_id, SUMMARY_HEADER)

      pagure_interface.update_issue(issue_id, vote_summary)

+ 

+     app.logger.debug('Saving voting info to database')

+     bug = pagure_interface.issue_to_bug(issue_id)

+     release = bug.milestone.release

+     milestones = Milestone.query.filter_by(release=release)

+     for milestone in milestones:

+         # bugid is unique in milestone, thus .first()

+         milestone_bug = Bug.query.filter_by(milestone=milestone, bugid=bug.bugid).first()

+         if not milestone_bug:

+             # bug is not present in given milestone

+             continue

+ 

+         milestone_votes = {}

+         # go through all trackers, e.g. bug can be proposed as beta blocker,

+         # but can receive final blocker votes, we want to see it in IRC format

+         for tracker_name, tracker in trackers.items():

+             votes = tracker.enumerate_votes()

+ 

+             # drop comment id

+             milestone_votes[tracker_name] = {

+                 "-1": [person_vote[0] for person_vote in votes["-1"]],

+                 "0": [person_vote[0] for person_vote in votes["0"]],

+                 "+1": [person_vote[0] for person_vote in votes["+1"]],

+             }

+ 

+         milestone_bug.votes = json.dumps(milestone_votes)

+         db.session.add(milestone_bug)

+         db.session.commit()

@@ -24,11 +24,14 @@ 

      return create_issue(title, content)

  

  

- def update_issue(issue_id, vote_summary):

-     update_issue_url = app.config['PAGURE_API'] + app.config['PAGURE_REPO'] + '/issue/%s' % issue_id

+ def issue_to_bug(issue_id):

      discussion_link = app.config['PAGURE_URL'] + app.config['PAGURE_REPO'] + "/issue/%s" % issue_id

+     return Bug.query.filter_by(discussion_link=discussion_link).first()

+ 

  

-     bug = Bug.query.filter_by(discussion_link=discussion_link).first()

+ def update_issue(issue_id, vote_summary):

+     update_issue_url = app.config['PAGURE_API'] + app.config['PAGURE_REPO'] + '/issue/%s' % issue_id

+     bug = issue_to_bug(issue_id)

  

      bug_url = app.config['BUGZILLA_URL'] + "show_bug.cgi?id=%s" % bug.bugid

      title = Template(app.config['PAGURE_DISCUSSION_TITLE']).safe_substitute(

Build succeeded.

Awesome, I'll have a look, hopefully today.

Build succeeded.

Ooof, it took me over an hour to digest this. It is very hard to read with all those nested structures, string keywords, and no documentation or an example value. (Please, next time at least document the structure of Release.votes, it would've saved me a lot of time.)

The most important thing seems to be that all the votes for all bugs related to a certain release are now stored in a single database field under Release.votes in a fat json. Is that correct? I find it a bit scary, to be honest. If anything goes wrong, e.g. there's some corruption in the json, then all the votes are gone. We rewrite the full json with any change in any bug. I assumed we could do it the same way as with discussion links, because it's the same use case, isn't? We want the same discussion link for all bugs with the same bugid related to the same release. And we want the same voting info for all bugs with the same bugid related to the same release. It's the same thing, right? Or am I missing something? We could reuse some of the existing code used for deduplication+updating all relevant bugs with the same information, just extend it to voting info. The jsons would be stored as Bug.votes and if there was some error, it would only affect that particular bug information. What are your thoughts on that?

I'll continue the review by commenting on particular code snippets.

Just by reading voting_tuple(), I had a hard time understanding what exactly this does, and how it is different from voting_info(). Would it make sense to rename these to voting_info_web() and voting_info_irc() (or similar)? And provide a short docstring, please?

Furthermore, I'm not exactly sure why these functions exist. Why don't you pre-compute all of this for all bugz retrieved and pass it as a json to the frontend? Isn't everything in bugz displayed anyway? It seems you just pass values to the frontend so that the frontend can pass them back to the backend and retrieve the data it wanted. That's why you needed to pass milestone_version as a new variable. But if there's no disadvantage, you can simply pass all the computed votes immediately, and you can avoid creating new variables new milestone_version completely (and it becomes easier to read without all of those back-and-forth communication). Thoughts?

This is probably obvious to everyone familiar with Flask, but why does this return a dictionary which has a function as a value? I'm a little confused by this.

Almost the same dictionary is in tracker_summary() in pagure_bot.py. Can we please have it in a single place?

Renaming p to something more obvious would greatly help readability. Should it be a person_tuple perhaps?

This desperately needs format specification, thank you. You can reference to other methods, e.g. you can say that value X is the output of BugVoteTracker.enumerate_votes(), that's fine (which is already nicely documented).

Can we please merge this with the "Review" column? I was imaging an output like this:

(+2, 0, -1)
   Vote!

or

(+5, 1, -0)
  Discuss

Dict comprehensions, nice, I completely forgot and had to look it up :-)

1 new commit added

  • rework
2 months ago

Build succeeded.

3 new commits added

  • rework
  • Show discussion vote counts in web UI
  • Show discussion vote counts in IRC format
2 months ago

Build succeeded.

LGTM, please either add some comments to web_voting_info or change the blockerbugs/templates/blocker_list.html template as we discussed in the meeting.

rebased onto c25cc8b

a month ago

Build succeeded.

rebased onto 16bfb6f

a month ago

Build succeeded.

rebased onto b43c0db

a month ago

Build succeeded.

Commit b43c0db fixes this pull-request

Pull-Request has been merged by lbrabec

a month ago

Pull-Request has been merged by lbrabec

a month ago