Changeset 53133 in webkit


Ignore:
Timestamp:
Jan 12, 2010 4:03:21 AM (14 years ago)
Author:
eric@webkit.org
Message:

2010-01-12 Eric Seidel <eric@webkit.org>

Reviewed by Adam Barth.

bugzilla.py should have an Attachment object instead of passing around dictionaries
https://bugs.webkit.org/show_bug.cgi?id=31594

  • Scripts/webkitpy/bugzilla.py:
    • Add a new Attachment class, with accessor methods for all the necessary properties.
    • Update Bug to carry a pointer back to bugzilla (attachments need to access Bugzilla for committer validation and url())
    • Move reviewed_patches and commit_queued_patches out of Bugzilla custom methods and onto Bug
    • Move committer validation logic into its own class.
    • Committer rejection is only used in one place. Make the new Bug reviewed_patches and commit_queued_patches handle the common case (of returning "reviewer" or "committer" as None), and let CommitterValidation handle the case where we want to reject patches in bugzilla.
    • Simplify fetch_patches_from_commit_queue now that committer validation is simpler.
    • Make all self.bugzilla.fetch_bug access go through BugzillaQueries._fetch_bug.
    • Mark set_flag_on_attachment as non-private to denote that CommitterValidation depends on it.
    • Move fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug logic onto the Bug class.
  • Scripts/webkitpy/bugzilla_unittest.py:
    • Move test_flag_permission_rejection_message into a new CommitterValidationTest class.
  • Scripts/webkitpy/commands/download.py:
    • Store "bug_id" in state instead of making a fake patch object.
    • Update to use Attachment and Bug objects.
  • Scripts/webkitpy/commands/download_unittest.py:
    • Update expected results now that our testing framework covers more code.
  • Scripts/webkitpy/commands/early_warning_system.py: Update to use new Attachment class.
  • Scripts/webkitpy/commands/queries.py: Remove unused ReviewedPatches class.
  • Scripts/webkitpy/commands/queries_unittest.py: ditto.
  • Scripts/webkitpy/commands/queues.py: Update to use new Attachment and CommitterValidator classes.
  • Scripts/webkitpy/commands/queuestest.py: ditto.
  • Scripts/webkitpy/commands/upload.py: ditto.
  • Scripts/webkitpy/mock_bugzillatool.py:
    • Now that more logic has moved into Attachment and Bug, we have to actually provide real reviewer emails as well as real reviewer flags.
    • Update mock methods to return Attachment objects.
  • Scripts/webkitpy/scm.py: Update to use Attachment class.
  • Scripts/webkitpy/scm_unittest.py: Update to use Attachment class.
  • Scripts/webkitpy/statusserver.py: ditto.
  • Scripts/webkitpy/steps/applypatch.py: ditto.
  • Scripts/webkitpy/steps/applypatchwithlocalcommit.py: ditto.
  • Scripts/webkitpy/steps/closebug.py: ditto.
  • Scripts/webkitpy/steps/closebugforlanddiff.py: Handle either statebug_id? or statepatch?.bug_id()
  • Scripts/webkitpy/steps/closepatch.py: Update to use Attachment class.
  • Scripts/webkitpy/steps/obsoletepatches.py: ditto.
  • Scripts/webkitpy/steps/updatechangelogswithreviewer.py: ditto.
Location:
trunk/WebKitTools
Files:
22 edited

Legend:

Unmodified
Added
Removed
  • trunk/WebKitTools/ChangeLog

    r53130 r53133  
     12010-01-12  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        bugzilla.py should have an Attachment object instead of passing around dictionaries
     6        https://bugs.webkit.org/show_bug.cgi?id=31594
     7
     8        * Scripts/webkitpy/bugzilla.py:
     9         - Add a new Attachment class, with accessor methods for all the necessary properties.
     10         - Update Bug to carry a pointer back to bugzilla (attachments need to access Bugzilla for committer validation and url())
     11         - Move reviewed_patches and commit_queued_patches out of Bugzilla custom methods and onto Bug
     12         - Move committer validation logic into its own class.
     13         - Committer rejection is only used in one place.  Make the new Bug reviewed_patches and commit_queued_patches
     14           handle the common case (of returning "reviewer" or "committer" as None), and let CommitterValidation handle
     15           the case where we want to reject patches in bugzilla.
     16         - Simplify fetch_patches_from_commit_queue now that committer validation is simpler.
     17         - Make all self.bugzilla.fetch_bug access go through BugzillaQueries._fetch_bug.
     18         - Mark set_flag_on_attachment as non-private to denote that CommitterValidation depends on it.
     19         - Move fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug logic onto the Bug class.
     20        * Scripts/webkitpy/bugzilla_unittest.py:
     21         - Move test_flag_permission_rejection_message into a new CommitterValidationTest class.
     22        * Scripts/webkitpy/commands/download.py:
     23         - Store "bug_id" in state instead of making a fake patch object.
     24         - Update to use Attachment and Bug objects.
     25        * Scripts/webkitpy/commands/download_unittest.py:
     26         - Update expected results now that our testing framework covers more code.
     27        * Scripts/webkitpy/commands/early_warning_system.py: Update to use new Attachment class.
     28        * Scripts/webkitpy/commands/queries.py: Remove unused ReviewedPatches class.
     29        * Scripts/webkitpy/commands/queries_unittest.py: ditto.
     30        * Scripts/webkitpy/commands/queues.py: Update to use new Attachment and CommitterValidator classes.
     31        * Scripts/webkitpy/commands/queuestest.py: ditto.
     32        * Scripts/webkitpy/commands/upload.py: ditto.
     33        * Scripts/webkitpy/mock_bugzillatool.py:
     34         - Now that more logic has moved into Attachment and Bug, we have to actually
     35           provide real reviewer emails as well as real reviewer flags.
     36         - Update mock methods to return Attachment objects.
     37        * Scripts/webkitpy/scm.py: Update to use Attachment class.
     38        * Scripts/webkitpy/scm_unittest.py: Update to use Attachment class.
     39        * Scripts/webkitpy/statusserver.py: ditto.
     40        * Scripts/webkitpy/steps/applypatch.py: ditto.
     41        * Scripts/webkitpy/steps/applypatchwithlocalcommit.py: ditto.
     42        * Scripts/webkitpy/steps/closebug.py: ditto.
     43        * Scripts/webkitpy/steps/closebugforlanddiff.py: Handle either state["bug_id"] or state["patch"].bug_id()
     44        * Scripts/webkitpy/steps/closepatch.py: Update to use Attachment class.
     45        * Scripts/webkitpy/steps/obsoletepatches.py: ditto.
     46        * Scripts/webkitpy/steps/updatechangelogswithreviewer.py: ditto.
     47
    1482010-01-12  Adam Barth  <abarth@webkit.org>
    249
  • trunk/WebKitTools/Scripts/webkitpy/bugzilla.py

    r53121 r53133  
    6060
    6161
     62class Attachment(object):
     63    def __init__(self, attachment_dictionary, bug):
     64        self._attachment_dictionary = attachment_dictionary
     65        self._bug = bug
     66        self._reviewer = None
     67        self._committer = None
     68
     69    def _bugzilla(self):
     70        return self._bug._bugzilla
     71
     72    def id(self):
     73        return int(self._attachment_dictionary.get("id"))
     74
     75    def attacher_is_committer(self):
     76        return self._bugzilla.committers.committer_by_email(patch.attacher_email())
     77
     78    def attacher_email(self):
     79        return self._attachment_dictionary.get("attacher_email")
     80
     81    def bug(self):
     82        return self._bug
     83
     84    def bug_id(self):
     85        return int(self._attachment_dictionary.get("bug_id"))
     86
     87    def is_patch(self):
     88        return not not self._attachment_dictionary.get("is_patch")
     89
     90    def is_obsolete(self):
     91        return not not self._attachment_dictionary.get("is_obsolete")
     92
     93    def name(self):
     94        return self._attachment_dictionary.get("name")
     95
     96    def review(self):
     97        return self._attachment_dictionary.get("review")
     98
     99    def commit_queue(self):
     100        return self._attachment_dictionary.get("commit-queue")
     101
     102    def url(self):
     103        # FIXME: This should just return self._bugzilla().attachment_url_for_id(self.id()).  scm_unittest.py depends on the current behavior.
     104        return self._attachment_dictionary.get("url")
     105
     106    def _validate_flag_value(self, flag):
     107        email = self._attachment_dictionary.get("%s_email" % flag)
     108        if not email:
     109            return None
     110        committer = getattr(self._bugzilla().committers, "%s_by_email" % flag)(email)
     111        if committer:
     112            return committer
     113        log("Warning, attachment %s on bug %s has invalid %s (%s)" % (self._attachment_dictionary['id'], self._attachment_dictionary['bug_id'], flag, email))
     114
     115    def reviewer(self):
     116        if not self._reviewer:
     117            self._reviewer = self._validate_flag_value("reviewer")
     118        return self._reviewer
     119
     120    def committer(self):
     121        if not self._committer:
     122            self._committer = self._validate_flag_value("committer")
     123        return self._committer
     124
     125
    62126# FIXME: This class is kinda a hack for now.  It exists so we have one place
    63127# to hold bug logic, even if much of the code deals with dictionaries still.
    64128class Bug(object):
    65     def __init__(self, bug_dictionary):
     129    def __init__(self, bug_dictionary, bugzilla):
    66130        self.bug_dictionary = bug_dictionary
     131        self._bugzilla = bugzilla
    67132
    68133    def assigned_to_email(self):
     
    71136    # Rarely do we actually want obsolete attachments
    72137    def attachments(self, include_obsolete=False):
    73         if include_obsolete:
    74             return self.bug_dictionary["attachments"][:] # Return a copy in either case.
    75         return [attachment for attachment in self.bug_dictionary["attachments"] if not attachment["is_obsolete"]]
     138        attachments = self.bug_dictionary["attachments"]
     139        if not include_obsolete:
     140            attachments = filter(lambda attachment: not attachment["is_obsolete"], attachments)
     141        return [Attachment(attachment, self) for attachment in attachments]
    76142
    77143    def patches(self, include_obsolete=False):
    78         return [patch for patch in self.attachments(include_obsolete) if patch["is_patch"]]
     144        return [patch for patch in self.attachments(include_obsolete) if patch.is_patch()]
    79145
    80146    def unreviewed_patches(self):
    81         return [patch for patch in self.patches() if patch.get("review") == "?"]
     147        return [patch for patch in self.patches() if patch.review() == "?"]
     148
     149    def reviewed_patches(self):
     150        # Checking reviewer() ensures that it was both reviewed and has a valid reviewer.
     151        return [patch for patch in self.patches() if patch.review() == "+" and patch.reviewer()]
     152
     153    def commit_queued_patches(self):
     154        # Checking committer() ensures that it was both commit-queue+'d and has a valid committer.
     155        return [patch for patch in self.patches() if patch.commit_queue() == "+" and patch.committer()]
    82156
    83157
     
    85159class BugzillaQueries(object):
    86160    def __init__(self, bugzilla):
    87         self.bugzilla = bugzilla
    88 
     161        self._bugzilla = bugzilla
     162
     163    # Note: _load_query and _fetch_bug are the only two methods which access self._bugzilla.
    89164    def _load_query(self, query):
    90         full_url = "%s%s" % (self.bugzilla.bug_server_url, query)
    91         return self.bugzilla.browser.open(full_url)
     165        full_url = "%s%s" % (self._bugzilla.bug_server_url, query)
     166        return self._bugzilla.browser.open(full_url)
     167
     168    def _fetch_bug(self, bug_id):
     169        return self._bugzilla.fetch_bug(bug_id)
     170
    92171
    93172    def _fetch_bug_ids_advanced_query(self, query):
     
    111190
    112191    def fetch_patches_from_pending_commit_list(self):
    113         # FIXME: This should not have to go through self.bugzilla
    114         return sum([self.bugzilla.fetch_reviewed_patches_from_bug(bug_id) for bug_id in self.fetch_bug_ids_from_pending_commit_list()], [])
     192        return sum([self._fetch_bug(bug_id).reviewed_patches() for bug_id in self.fetch_bug_ids_from_pending_commit_list()], [])
    115193
    116194    def fetch_bug_ids_from_commit_queue(self):
     
    118196        return self._fetch_bug_ids_advanced_query(commit_queue_url)
    119197
    120     def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
    121         # FIXME: Once reject_invalid_patches is moved out of this function this becomes a simple list comprehension using fetch_bug_ids_from_commit_queue.
    122         patches_to_land = []
    123         for bug_id in self.fetch_bug_ids_from_commit_queue():
    124             # FIXME: This should not have to go through self.bugzilla
    125             patches = self.bugzilla.fetch_commit_queue_patches_from_bug(bug_id, reject_invalid_patches)
    126             patches_to_land += patches
    127         return patches_to_land
     198    def fetch_patches_from_commit_queue(self):
     199        return sum([self._fetch_bug(bug_id).commit_queued_patches() for bug_id in self.fetch_bug_ids_from_commit_queue()], [])
    128200
    129201    def _fetch_bug_ids_from_review_queue(self):
     
    132204
    133205    def fetch_patches_from_review_queue(self, limit=None):
    134         # FIXME: We should probably have a self.fetch_bug to minimize the number of self.bugzilla calls.
    135         return sum([self.bugzilla.fetch_bug(bug_id).unreviewed_patches() for bug_id in self._fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
     206        return sum([self._fetch_bug(bug_id).unreviewed_patches() for bug_id in self._fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
    136207
    137208    # FIXME: Why do we have both fetch_patches_from_review_queue and fetch_attachment_ids_from_review_queue??
     
    142213
    143214
    144 class Bugzilla(object):
    145     def __init__(self, dryrun=False, committers=CommitterList()):
    146         self.dryrun = dryrun
    147         self.authenticated = False
    148         self.queries = BugzillaQueries(self)
    149 
    150         # FIXME: We should use some sort of Browser mock object when in dryrun mode (to prevent any mistakes).
    151         self.browser = Browser()
    152         # Ignore bugs.webkit.org/robots.txt until we fix it to allow this script
    153         self.browser.set_handle_robots(False)
    154         self.committers = committers
    155 
    156     # FIXME: Much of this should go into some sort of config module:
    157     bug_server_host = "bugs.webkit.org"
    158     bug_server_regex = "https?://%s/" % re.sub('\.', '\\.', bug_server_host)
    159     bug_server_url = "https://%s/" % bug_server_host
    160     unassigned_email = "webkit-unassigned@lists.webkit.org"
    161 
    162     def bug_url_for_bug_id(self, bug_id, xml=False):
    163         content_type = "&ctype=xml" if xml else ""
    164         return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url, bug_id, content_type)
    165 
    166     def short_bug_url_for_bug_id(self, bug_id):
    167         return "http://webkit.org/b/%s" % bug_id
    168 
    169     def attachment_url_for_id(self, attachment_id, action="view"):
    170         action_param = ""
    171         if action and action != "view":
    172             action_param = "&action=%s" % action
    173         return "%sattachment.cgi?id=%s%s" % (self.bug_server_url, attachment_id, action_param)
    174 
    175     def _parse_attachment_flag(self, element, flag_name, attachment, result_key):
    176         flag = element.find('flag', attrs={'name' : flag_name})
    177         if flag:
    178             attachment[flag_name] = flag['status']
    179             if flag['status'] == '+':
    180                 attachment[result_key] = flag['setter']
    181 
    182     def _parse_attachment_element(self, element, bug_id):
    183         attachment = {}
    184         attachment['bug_id'] = bug_id
    185         attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1")
    186         attachment['is_patch'] = (element.has_key('ispatch') and element['ispatch'] == "1")
    187         attachment['id'] = int(element.find('attachid').string)
    188         attachment['url'] = self.attachment_url_for_id(attachment['id'])
    189         attachment['name'] = unicode(element.find('desc').string)
    190         attachment['attacher_email'] = str(element.find('attacher').string)
    191         attachment['type'] = str(element.find('type').string)
    192         self._parse_attachment_flag(element, 'review', attachment, 'reviewer_email')
    193         self._parse_attachment_flag(element, 'commit-queue', attachment, 'committer_email')
    194         return attachment
    195 
    196     def _parse_bug_page(self, page):
    197         soup = BeautifulSoup(page)
    198         bug = {}
    199         bug["id"] = int(soup.find("bug_id").string)
    200         bug["title"] = unicode(soup.find("short_desc").string)
    201         bug["reporter_email"] = str(soup.find("reporter").string)
    202         bug["assigned_to_email"] = str(soup.find("assigned_to").string)
    203         bug["cc_emails"] = [str(element.string) for element in soup.findAll('cc')]
    204         bug["attachments"] = [self._parse_attachment_element(element, bug["id"]) for element in soup.findAll('attachment')]
    205         return bug
    206 
    207     # Makes testing fetch_*_from_bug() possible until we have a better BugzillaNetwork abstration.
    208     def _fetch_bug_page(self, bug_id):
    209         bug_url = self.bug_url_for_bug_id(bug_id, xml=True)
    210         log("Fetching: %s" % bug_url)
    211         return self.browser.open(bug_url)
    212 
    213     def fetch_bug_dictionary(self, bug_id):
    214         return self._parse_bug_page(self._fetch_bug_page(bug_id))
    215 
    216     # FIXME: A BugzillaCache object should provide all these fetch_ methods.
    217     def fetch_bug(self, bug_id):
    218         return Bug(self.fetch_bug_dictionary(bug_id))
    219 
    220     def _parse_bug_id_from_attachment_page(self, page):
    221         up_link = BeautifulSoup(page).find('link', rel='Up') # The "Up" relation happens to point to the bug.
    222         if not up_link:
    223             return None # This attachment does not exist (or you don't have permissions to view it).
    224         match = re.search("show_bug.cgi\?id=(?P<bug_id>\d+)", up_link['href'])
    225         return int(match.group('bug_id'))
    226 
    227     def bug_id_for_attachment_id(self, attachment_id):
    228         attachment_url = self.attachment_url_for_id(attachment_id, 'edit')
    229         log("Fetching: %s" % attachment_url)
    230         page = self.browser.open(attachment_url)
    231         return self._parse_bug_id_from_attachment_page(page)
    232 
    233     # This should really return an Attachment object
    234     # which can lazily fetch any missing data.
    235     def fetch_attachment(self, attachment_id):
    236         # We could grab all the attachment details off of the attachment edit page
    237         # but we already have working code to do so off of the bugs page, so re-use that.
    238         bug_id = self.bug_id_for_attachment_id(attachment_id)
    239         if not bug_id:
    240             return None
    241         for attachment in self.fetch_bug(bug_id).attachments(include_obsolete=True):
    242             # FIXME: Once we have a real Attachment class we shouldn't paper over this possible comparison failure
    243             # and we should remove the int() == int() hacks and leave it just ==.
    244             if int(attachment['id']) == int(attachment_id):
    245                 self._validate_committer_and_reviewer(attachment)
    246                 return attachment
    247         return None # This should never be hit.
    248 
    249     # fetch_patches_from_bug exists until we expose a Bug class outside of bugzilla.py
    250     def fetch_patches_from_bug(self, bug_id):
    251         return self.fetch_bug(bug_id).patches()
     215class CommitterValidator(object):
     216    def __init__(self, bugzilla):
     217        self._bugzilla = bugzilla
    252218
    253219    # _view_source_link belongs in some sort of webkit_config.py module.
     
    268234        return rejection_message
    269235
    270     def _validate_setter_email(self, patch, result_key, lookup_function, rejection_function, reject_invalid_patches):
    271         setter_email = patch.get(result_key + '_email')
    272         if not setter_email:
     236    def _validate_setter_email(self, patch, result_key, rejection_function):
     237        committer = getattr(patch, result_key)()
     238        # If the flag is set, and we don't recognize the setter, reject the flag!
     239        if patch._attachment_dictionary.get("%s_email" % result_key) and not committer:
     240            rejection_function(patch.id(), self._flag_permission_rejection_message(setter_email, result_key))
     241            return False
     242        return True
     243
     244    def patches_after_rejecting_invalid_commiters_and_reviewers(self, patches):
     245        validated_patches = []
     246        for patch in patches:
     247            if self._validate_setter_email(patch, "reviewer", self.reject_patch_from_review_queue) and self._validate_setter_email(patch, "committer", self.reject_patch_from_commit_queue):
     248                validated_patches.append(patch)
     249        return validated_patches
     250
     251    def reject_patch_from_commit_queue(self, attachment_id, additional_comment_text=None):
     252        comment_text = "Rejecting patch %s from commit-queue." % attachment_id
     253        self._bugzilla.set_flag_on_attachment(attachment_id, 'commit-queue', '-', comment_text, additional_comment_text)
     254
     255    def reject_patch_from_review_queue(self, attachment_id, additional_comment_text=None):
     256        comment_text = "Rejecting patch %s from review queue." % attachment_id
     257        self._bugzilla.set_flag_on_attachment(attachment_id, 'review', '-', comment_text, additional_comment_text)
     258
     259
     260class Bugzilla(object):
     261    def __init__(self, dryrun=False, committers=CommitterList()):
     262        self.dryrun = dryrun
     263        self.authenticated = False
     264        self.queries = BugzillaQueries(self)
     265        self.committers = committers
     266
     267        # FIXME: We should use some sort of Browser mock object when in dryrun mode (to prevent any mistakes).
     268        self.browser = Browser()
     269        # Ignore bugs.webkit.org/robots.txt until we fix it to allow this script
     270        self.browser.set_handle_robots(False)
     271
     272    # FIXME: Much of this should go into some sort of config module:
     273    bug_server_host = "bugs.webkit.org"
     274    bug_server_regex = "https?://%s/" % re.sub('\.', '\\.', bug_server_host)
     275    bug_server_url = "https://%s/" % bug_server_host
     276    unassigned_email = "webkit-unassigned@lists.webkit.org"
     277
     278    def bug_url_for_bug_id(self, bug_id, xml=False):
     279        content_type = "&ctype=xml" if xml else ""
     280        return "%sshow_bug.cgi?id=%s%s" % (self.bug_server_url, bug_id, content_type)
     281
     282    def short_bug_url_for_bug_id(self, bug_id):
     283        return "http://webkit.org/b/%s" % bug_id
     284
     285    def attachment_url_for_id(self, attachment_id, action="view"):
     286        action_param = ""
     287        if action and action != "view":
     288            action_param = "&action=%s" % action
     289        return "%sattachment.cgi?id=%s%s" % (self.bug_server_url, attachment_id, action_param)
     290
     291    def _parse_attachment_flag(self, element, flag_name, attachment, result_key):
     292        flag = element.find('flag', attrs={'name' : flag_name})
     293        if flag:
     294            attachment[flag_name] = flag['status']
     295            if flag['status'] == '+':
     296                attachment[result_key] = flag['setter']
     297
     298    def _parse_attachment_element(self, element, bug_id):
     299        attachment = {}
     300        attachment['bug_id'] = bug_id
     301        attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1")
     302        attachment['is_patch'] = (element.has_key('ispatch') and element['ispatch'] == "1")
     303        attachment['id'] = int(element.find('attachid').string)
     304        # FIXME: No need to parse out the url here.
     305        attachment['url'] = self.attachment_url_for_id(attachment['id'])
     306        attachment['name'] = unicode(element.find('desc').string)
     307        attachment['attacher_email'] = str(element.find('attacher').string)
     308        attachment['type'] = str(element.find('type').string)
     309        self._parse_attachment_flag(element, 'review', attachment, 'reviewer_email')
     310        self._parse_attachment_flag(element, 'commit-queue', attachment, 'committer_email')
     311        return attachment
     312
     313    def _parse_bug_page(self, page):
     314        soup = BeautifulSoup(page)
     315        bug = {}
     316        bug["id"] = int(soup.find("bug_id").string)
     317        bug["title"] = unicode(soup.find("short_desc").string)
     318        bug["reporter_email"] = str(soup.find("reporter").string)
     319        bug["assigned_to_email"] = str(soup.find("assigned_to").string)
     320        bug["cc_emails"] = [str(element.string) for element in soup.findAll('cc')]
     321        bug["attachments"] = [self._parse_attachment_element(element, bug["id"]) for element in soup.findAll('attachment')]
     322        return bug
     323
     324    # Makes testing fetch_*_from_bug() possible until we have a better BugzillaNetwork abstration.
     325    def _fetch_bug_page(self, bug_id):
     326        bug_url = self.bug_url_for_bug_id(bug_id, xml=True)
     327        log("Fetching: %s" % bug_url)
     328        return self.browser.open(bug_url)
     329
     330    def fetch_bug_dictionary(self, bug_id):
     331        return self._parse_bug_page(self._fetch_bug_page(bug_id))
     332
     333    # FIXME: A BugzillaCache object should provide all these fetch_ methods.
     334    def fetch_bug(self, bug_id):
     335        return Bug(self.fetch_bug_dictionary(bug_id), self)
     336
     337    def _parse_bug_id_from_attachment_page(self, page):
     338        up_link = BeautifulSoup(page).find('link', rel='Up') # The "Up" relation happens to point to the bug.
     339        if not up_link:
     340            return None # This attachment does not exist (or you don't have permissions to view it).
     341        match = re.search("show_bug.cgi\?id=(?P<bug_id>\d+)", up_link['href'])
     342        return int(match.group('bug_id'))
     343
     344    def bug_id_for_attachment_id(self, attachment_id):
     345        attachment_url = self.attachment_url_for_id(attachment_id, 'edit')
     346        log("Fetching: %s" % attachment_url)
     347        page = self.browser.open(attachment_url)
     348        return self._parse_bug_id_from_attachment_page(page)
     349
     350    # FIXME: This should just return Attachment(id), which should be able to lazily fetch needed data.
     351    def fetch_attachment(self, attachment_id):
     352        # We could grab all the attachment details off of the attachment edit page
     353        # but we already have working code to do so off of the bugs page, so re-use that.
     354        bug_id = self.bug_id_for_attachment_id(attachment_id)
     355        if not bug_id:
    273356            return None
    274 
    275         committer = lookup_function(setter_email)
    276         if committer:
    277             patch[result_key] = committer.full_name
    278             return patch[result_key]
    279 
    280         if reject_invalid_patches:
    281             rejection_function(patch['id'], self._flag_permission_rejection_message(setter_email, result_key))
    282         else:
    283             log("Warning, attachment %s on bug %s has invalid %s (%s)" % (patch['id'], patch['bug_id'], result_key, setter_email))
    284         return None
    285 
    286     def _validate_reviewer(self, patch, reject_invalid_patches):
    287         return self._validate_setter_email(patch, 'reviewer', self.committers.reviewer_by_email, self.reject_patch_from_review_queue, reject_invalid_patches)
    288 
    289     def _validate_committer(self, patch, reject_invalid_patches):
    290         return self._validate_setter_email(patch, 'committer', self.committers.committer_by_email, self.reject_patch_from_commit_queue, reject_invalid_patches)
    291 
    292     # FIXME: This is a hack until we have a real Attachment object.
    293     # _validate_committer and _validate_reviewer fill in the 'reviewer' and 'committer'
    294     # keys which other parts of the code expect to be filled in.
    295     def _validate_committer_and_reviewer(self, patch):
    296         self._validate_reviewer(patch, reject_invalid_patches=False)
    297         self._validate_committer(patch, reject_invalid_patches=False)
    298 
    299     # FIXME: fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug
    300     # should share more code and use list comprehensions.
    301     def fetch_reviewed_patches_from_bug(self, bug_id, reject_invalid_patches=False):
    302         reviewed_patches = []
    303         for attachment in self.fetch_bug(bug_id).attachments():
    304             if self._validate_reviewer(attachment, reject_invalid_patches):
    305                 reviewed_patches.append(attachment)
    306         return reviewed_patches
    307 
    308     def fetch_commit_queue_patches_from_bug(self, bug_id, reject_invalid_patches=False):
    309         commit_queue_patches = []
    310         for attachment in self.fetch_reviewed_patches_from_bug(bug_id, reject_invalid_patches):
    311             if self._validate_committer(attachment, reject_invalid_patches):
    312                 commit_queue_patches.append(attachment)
    313         return commit_queue_patches
     357        for attachment in self.fetch_bug(bug_id).attachments(include_obsolete=True):
     358            if attachment.id() == int(attachment_id):
     359                return attachment
     360        return None # This should never be hit.
    314361
    315362    def authenticate(self):
     
    447494
    448495    # FIXME: We need a way to test this on a live bugzilla instance.
    449     def _set_flag_on_attachment(self, attachment_id, flag_name, flag_value, comment_text, additional_comment_text):
     496    def set_flag_on_attachment(self, attachment_id, flag_name, flag_value, comment_text, additional_comment_text):
    450497        self.authenticate()
    451498
     
    462509        self._find_select_element_for_flag(flag_name).value = (flag_value,)
    463510        self.browser.submit()
    464 
    465     def reject_patch_from_commit_queue(self, attachment_id, additional_comment_text=None):
    466         comment_text = "Rejecting patch %s from commit-queue." % attachment_id
    467         self._set_flag_on_attachment(attachment_id, 'commit-queue', '-', comment_text, additional_comment_text)
    468 
    469     def reject_patch_from_review_queue(self, attachment_id, additional_comment_text=None):
    470         comment_text = "Rejecting patch %s from review queue." % attachment_id
    471         self._set_flag_on_attachment(attachment_id, 'review', '-', comment_text, additional_comment_text)
    472511
    473512    # FIXME: All of these bug editing methods have a ridiculous amount of copy/paste code.
  • trunk/WebKitTools/Scripts/webkitpy/bugzilla_unittest.py

    r52907 r53133  
    3030
    3131from webkitpy.committers import CommitterList, Reviewer, Committer
    32 from webkitpy.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id
     32from webkitpy.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id, CommitterValidator
    3333from webkitpy.outputcapture import OutputCapture
    3434from webkitpy.mock import Mock
     
    4949    def submit(self):
    5050        pass
     51
     52class CommitterValidatorTest(unittest.TestCase):
     53    def test_flag_permission_rejection_message(self):
     54        validator = CommitterValidator(bugzilla=None)
     55        expected_messsage="""foo@foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.
     56
     57- If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
     58
     59- If you have review rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your review rights."""
     60        self.assertEqual(validator._flag_permission_rejection_message("foo@foo.com", "review"), expected_messsage)
    5161
    5262
     
    179189        "attachments" : [{
    180190            'name': u'Patch',
    181             'url': 'https://bugs.webkit.org/attachment.cgi?id=45548',
     191            'url' : "https://bugs.webkit.org/attachment.cgi?id=45548",
    182192            'is_obsolete': False,
    183193            'review': '?',
     
    231241        expected_stderr = "Adding ['adam@example.com'] to the CC list for bug 42\n"
    232242        OutputCapture().assert_outputs(self, bugzilla.add_cc_to_bug, [42, ["adam@example.com"]], expected_stderr=expected_stderr)
    233 
    234     def test_flag_permission_rejection_message(self):
    235         bugzilla = Bugzilla()
    236         expected_messsage="""foo@foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.
    237 
    238 - If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
    239 
    240 - If you have review rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your review rights."""
    241         self.assertEqual(bugzilla._flag_permission_rejection_message("foo@foo.com", "review"), expected_messsage)
    242243
    243244
  • trunk/WebKitTools/Scripts/webkitpy/commands/download.py

    r53105 r53133  
    8787    def _prepare_state(self, options, args, tool):
    8888        return {
    89             "patch" : {
    90                 "id" : None,
    91                 "bug_id" : (args and args[0]) or parse_bug_id(tool.scm().create_patch()),
    92             }
     89            "bug_id" : (args and args[0]) or parse_bug_id(tool.scm().create_patch()),
    9390        }
    9491
     
    105102        bugs_to_patches = {}
    106103        for patch in patches:
    107             bug_id = patch["bug_id"]
    108             bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []) + [patch]
     104            bugs_to_patches[patch.bug_id()] = bugs_to_patches.get(patch.bug_id(), []) + [patch]
    109105        return bugs_to_patches
    110106
     
    149145        all_patches = []
    150146        for bug_id in args:
    151             patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
     147            patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
    152148            log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
    153149            all_patches += patches
  • trunk/WebKitTools/Scripts/webkitpy/commands/download_unittest.py

    r53018 r53133  
    8383
    8484    def test_land_attachment(self):
    85         expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nBuilding WebKit\nRunning Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\n"
     85        # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
     86        expected_stderr = """Processing 1 patch from 1 bug.
     87Updating working directory
     88Processing patch 197 from bug 42.
     89Building WebKit
     90Running Python unit tests
     91Running Perl unit tests
     92Running JavaScriptCore tests
     93Running run-webkit-tests
     94Not closing bug 42 as attachment 197 has review=+.  Assuming there are more patches to land from this bug.
     95"""
    8696        self.assert_execute_outputs(LandAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr)
    8797
    8898    def test_land_patches(self):
     99        # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
    89100        expected_stderr = """2 reviewed patches found on bug 42.
    90101Processing 2 patches from 1 bug.
     
    96107Running JavaScriptCore tests
    97108Running run-webkit-tests
     109Not closing bug 42 as attachment 197 has review=+.  Assuming there are more patches to land from this bug.
    98110Updating working directory
    99111Processing patch 128 from bug 42.
     
    103115Running JavaScriptCore tests
    104116Running run-webkit-tests
     117Not closing bug 42 as attachment 197 has review=+.  Assuming there are more patches to land from this bug.
    105118"""
    106119        self.assert_execute_outputs(LandFromBug(), [42], options=self._default_options(), expected_stderr=expected_stderr)
  • trunk/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py

    r53130 r53133  
    6060            "--parent-command=%s" % self.name,
    6161            "--no-update",
    62             patch["id"]])
     62            patch.id()])
    6363
    6464    @classmethod
     
    6969            QueueEngine.exit_after_handled_error(script_error)
    7070        results_link = tool.status_server.results_url_for_status(status_id)
    71         message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"]["id"], cls.port_name, results_link)
    72         tool.bugs.post_comment_to_bug(state["patch"]["bug_id"], message, cc=cls.watchers)
     71        message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"].id(), cls.port_name, results_link)
     72        tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
    7373        exit(1)
    7474
     
    104104
    105105    def process_work_item(self, patch):
    106         if not self._committers.committer_by_email(patch["attacher_email"]):
     106        if not self._committers.committer_by_email(patch.attacher_email()):
    107107            self._did_error(patch, "%s cannot process patches from non-committers :(" % self.name)
    108108            return
  • trunk/WebKitTools/Scripts/webkitpy/commands/queries.py

    r53066 r53133  
    5656        log("Patches in commit queue:")
    5757        for patch in patches:
    58             print "%s" % patch["url"]
     58            print patch.url()
    5959
    6060
     
    7070    @staticmethod
    7171    def _needs_commit_queue(patch):
    72         commit_queue_flag = patch.get("commit-queue")
    73         if (commit_queue_flag and commit_queue_flag == '+'): # If it's already cq+, ignore the patch.
    74             log("%s already has cq=%s" % (patch["id"], commit_queue_flag))
     72        if patch.commit_queue() == "+": # If it's already cq+, ignore the patch.
     73            log("%s already has cq=%s" % (patch.id(), patch.commit_queue()))
    7574            return False
    7675
    7776        # We only need to worry about patches from contributers who are not yet committers.
    78         committer_record = CommitterList().committer_by_email(patch["attacher_email"])
     77        committer_record = CommitterList().committer_by_email(patch.attacher_email())
    7978        if committer_record:
    80             log("%s committer = %s" % (patch["id"], committer_record))
     79            log("%s committer = %s" % (patch.id(), committer_record))
    8180        return not committer_record
    8281
     
    8584        patches_needing_cq = filter(self._needs_commit_queue, patches)
    8685        if options.bugs:
    87             bugs_needing_cq = map(lambda patch: patch['bug_id'], patches_needing_cq)
     86            bugs_needing_cq = map(lambda patch: patch.bug_id(), patches_needing_cq)
    8887            bugs_needing_cq = sorted(set(bugs_needing_cq))
    8988            for bug_id in bugs_needing_cq:
     
    9190        else:
    9291            for patch in patches_needing_cq:
    93                 print "%s" % tool.bugs.attachment_url_for_id(patch["id"], action="edit")
     92                print "%s" % tool.bugs.attachment_url_for_id(patch.id(), action="edit")
    9493
    9594
     
    105104
    106105
    107 class ReviewedPatches(AbstractDeclarativeCommand):
    108     name = "reviewed-patches"
    109     help_text = "List r+'d patches on a bug"
    110     argument_names = "BUGID"
    111 
    112     def execute(self, options, args, tool):
    113         bug_id = args[0]
    114         patches_to_land = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
    115         for patch in patches_to_land:
    116             print "%s" % patch["url"]
    117 
    118 
    119106class TreeStatus(AbstractDeclarativeCommand):
    120107    name = "tree-status"
  • trunk/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py

    r52703 r53133  
    6060        self.assert_execute_outputs(PatchesToReview(), None, expected_stdout, expected_stderr)
    6161
    62     def test_reviewed_patches(self):
    63         expected_stdout = "http://example.com/197\nhttp://example.com/128\n"
    64         self.assert_execute_outputs(ReviewedPatches(), [42], expected_stdout)
    65 
    6662    def test_tree_status(self):
    6763        expected_stdout = "ok   : Builder1\nok   : Builder2\n"
  • trunk/WebKitTools/Scripts/webkitpy/commands/queues.py

    r53130 r53133  
    3636from StringIO import StringIO
    3737
     38from webkitpy.bugzilla import CommitterValidator
    3839from webkitpy.executive import ScriptError
    3940from webkitpy.grammar import pluralize
     
    8485
    8586    def work_item_log_path(self, patch):
    86         return os.path.join("%s-logs" % self.name, "%s.log" % patch["bug_id"])
     87        return os.path.join("%s-logs" % self.name, "%s.log" % patch.bug_id())
    8788
    8889    def begin_work_queue(self):
     
    141142    def begin_work_queue(self):
    142143        AbstractQueue.begin_work_queue(self)
     144        self.committer_validator = CommitterValidator(self.tool.bugs)
    143145
    144146    def next_work_item(self):
    145         patches = self.tool.bugs.queries.fetch_patches_from_commit_queue(reject_invalid_patches=True)
     147        patches = self.tool.bugs.queries.fetch_patches_from_commit_queue()
     148        patches = self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(patches)
    146149        if not patches:
    147150            self._update_status("Empty queue")
    148151            return None
    149152        # Only bother logging if we have patches in the queue.
    150         self.log_progress([patch['id'] for patch in patches])
     153        self.log_progress([patch.id() for patch in patches])
    151154        return patches[0]
    152155
     
    179182    def process_work_item(self, patch):
    180183        try:
    181             self._cc_watchers(patch["bug_id"])
     184            self._cc_watchers(patch.bug_id())
    182185            # We pass --no-update here because we've already validated
    183186            # that the current revision actually builds and passes the tests.
    184187            # If we update, we risk moving to a revision that doesn't!
    185             self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch["id"]])
     188            self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
    186189            self._did_pass(patch)
    187190        except ScriptError, e:
     
    190193
    191194    def handle_unexpected_error(self, patch, message):
    192         self.tool.bugs.reject_patch_from_commit_queue(patch["id"], message)
     195        self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
    193196
    194197    # StepSequenceErrorHandler methods
     
    204207    def handle_script_error(cls, tool, state, script_error):
    205208        status_id = cls._update_status_for_script_error(tool, state, script_error)
    206         tool.bugs.reject_patch_from_commit_queue(state["patch"]["id"], cls._error_message_for_bug(tool, status_id, script_error))
     209        validator = CommitterValidator(tool.bugs)
     210        validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
    207211
    208212
     
    269273
    270274    def _review_patch(self, patch):
    271         self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch["id"]])
     275        self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch.id()])
    272276
    273277    @classmethod
     
    277281        if is_svn_apply:
    278282            QueueEngine.exit_after_handled_error(script_error)
    279         message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"]["id"], cls.name, script_error.message_with_output(output_limit=3*1024))
    280         tool.bugs.post_comment_to_bug(state["patch"]["bug_id"], message, cc=cls.watchers)
     283        message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"].id(), cls.name, script_error.message_with_output(output_limit=3*1024))
     284        tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
    281285        exit(1)
  • trunk/WebKitTools/Scripts/webkitpy/commands/queuestest.py

    r52719 r53133  
    2929import unittest
    3030
     31from webkitpy.bugzilla import Attachment
    3132from webkitpy.mock import Mock
    3233from webkitpy.mock_bugzillatool import MockBugzillaTool
     
    4344
    4445class QueuesTest(unittest.TestCase):
    45     mock_work_item = {
     46    mock_work_item = Attachment({
    4647        "id" : 1234,
    4748        "bug_id" : 345,
    4849        "attacher_email": "adam@example.com",
    49     }
     50    }, None)
    5051
    5152    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, options=Mock(), tool=MockBugzillaTool()):
  • trunk/WebKitTools/Scripts/webkitpy/commands/upload.py

    r53120 r53133  
    6969
    7070        # FIXME: This should call a reviewed_patches() method on bug instead of re-fetching.
    71         reviewed_patches = self.tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
     71        reviewed_patches = self.tool.bugs.fetch_bug(bug_id).reviewed_patches()
    7272        if not reviewed_patches:
    7373            log("Bug %s has no non-obsolete patches, ignoring." % bug_id)
    7474            return
    7575        latest_patch = reviewed_patches[-1]
    76         attacher_email = latest_patch["attacher_email"]
     76        attacher_email = latest_patch.attacher_email()
    7777        committer = committers.committer_by_email(attacher_email)
    7878        if not committer:
     
    8080            return
    8181
    82         reassign_message = "Attachment %s was posted by a committer and has review+, assigning to %s for commit." % (latest_patch["id"], committer.full_name)
     82        reassign_message = "Attachment %s was posted by a committer and has review+, assigning to %s for commit." % (latest_patch.id(), committer.full_name)
    8383        self.tool.bugs.reassign_bug(bug_id, committer.bugzilla_email(), reassign_message)
    8484
  • trunk/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py

    r53105 r53133  
    3131from webkitpy.mock import Mock
    3232from webkitpy.scm import CommitMessage
    33 from webkitpy.bugzilla import Bug
     33from webkitpy.committers import CommitterList, Reviewer
     34from webkitpy.bugzilla import Bug, Attachment
    3435
    3536def _id_to_object_dictionary(*objects):
     
    4647    "is_obsolete" : False,
    4748    "is_patch" : True,
    48     "reviewer" : "Reviewer1",
     49    "review" : "+",
     50    "reviewer_email" : "foo@bar.com",
    4951    "attacher_email" : "Contributer1",
    5052}
     
    5557    "is_obsolete" : False,
    5658    "is_patch" : True,
    57     "reviewer" : "Reviewer2",
     59    "review" : "+",
     60    "reviewer_email" : "foo@bar.com",
    5861    "attacher_email" : "eric@webkit.org",
    5962}
     
    8386
    8487class MockBugzillaQueries(Mock):
     88    def __init__(self, bugzilla):
     89        Mock.__init__(self)
     90        self._bugzilla = bugzilla
     91
    8592    def fetch_bug_ids_from_commit_queue(self):
    8693        return [42, 75]
     
    9097
    9198    def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
    92         return [_patch1, _patch2]
     99        return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
    93100
    94101    def fetch_bug_ids_from_pending_commit_list(self):
     
    96103   
    97104    def fetch_patches_from_pending_commit_list(self):
    98         return [_patch1, _patch2]
    99 
    100 
     105        return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
     106
     107
     108# FIXME: Bugzilla is the wrong Mock-point.  Once we have a BugzillaNetwork class we should mock that instead.
     109# Most of this class is just copy/paste from Bugzilla.
    101110class MockBugzilla(Mock):
    102111    bug_server_url = "http://example.com"
     
    104113    bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3)
    105114    attachment_cache = _id_to_object_dictionary(_patch1, _patch2)
    106     queries = MockBugzillaQueries()
     115
     116    def __init__(self):
     117        Mock.__init__(self)
     118        self.queries = MockBugzillaQueries(self)
     119        self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo@bar.com")])
    107120
    108121    def fetch_bug(self, bug_id):
    109         return Bug(self.bug_cache.get(bug_id))
    110 
    111     def fetch_reviewed_patches_from_bug(self, bug_id):
    112         return self.fetch_patches_from_bug(bug_id) # Return them all for now.
     122        return Bug(self.bug_cache.get(bug_id), self)
    113123
    114124    def fetch_attachment(self, attachment_id):
    115         return self.attachment_cache[attachment_id] # This could be changed to .get() if we wish to allow failed lookups.
    116 
    117     # NOTE: Functions below this are direct copies from bugzilla.py
    118     def fetch_patches_from_bug(self, bug_id):
    119         return self.fetch_bug(bug_id).patches()
    120    
     125        attachment_dictionary = self.attachment_cache[attachment_id] # This could be changed to .get() if we wish to allow failed lookups.
     126        bug = self.fetch_bug(attachment_dictionary["bug_id"])
     127        for attachment in bug.attachments(include_obsolete=True):
     128            if attachment.id() == int(attachment_id):
     129                return attachment
     130
    121131    def bug_url_for_bug_id(self, bug_id):
    122132        return "%s/%s" % (self.bug_server_url, bug_id)
     
    125135        return self.bug_cache.get(bug_id)
    126136
    127     def attachment_url_for_id(self, attachment_id, action):
     137    def attachment_url_for_id(self, attachment_id, action="view"):
    128138        action_param = ""
    129139        if action and action != "view":
  • trunk/WebKitTools/Scripts/webkitpy/scm.py

    r52703 r53133  
    124124        # It's possible that the patch was not made from the root directory.
    125125        # We should detect and handle that case.
    126         curl_process = subprocess.Popen(['curl', '--location', '--silent', '--show-error', patch['url']], stdout=subprocess.PIPE)
     126        # FIXME: scm.py should not deal with fetching Attachment data.  Attachment should just have a .data() accessor.
     127        curl_process = subprocess.Popen(['curl', '--location', '--silent', '--show-error', patch.url()], stdout=subprocess.PIPE)
    127128        args = [self.script_path('svn-apply')]
    128         if patch.get('reviewer'):
    129             args += ['--reviewer', patch['reviewer']]
     129        if patch.reviewer():
     130            args += ['--reviewer', patch.reviewer().full_name]
    130131        if force:
    131132            args.append('--force')
  • trunk/WebKitTools/Scripts/webkitpy/scm_unittest.py

    r52703 r53133  
    4141from webkitpy.executive import Executive, run_command, ScriptError
    4242from webkitpy.scm import detect_scm_system, SCM, CheckoutNeedsUpdate, commit_error_handler
     43from webkitpy.bugzilla import Attachment # FIXME: This should not be needed
    4344
    4445# Eventually we will want to write tests which work for both scms. (like update_webkit, changed_files, etc.)
     
    169170        patch['bug_id'] = '12345'
    170171        patch['url'] = 'file://%s' % urllib.pathname2url(patch_path)
    171         return patch
     172        return Attachment(patch, None) # FIXME: This is a hack, scm.py shouldn't be fetching attachment data.
    172173
    173174    def _setup_webkittools_scripts_symlink(self, local_scm):
  • trunk/WebKitTools/Scripts/webkitpy/statusserver.py

    r53043 r53133  
    5555        if not patch:
    5656            return
    57         if patch.get('bug_id'):
    58             self.browser['bug_id'] = str(patch['bug_id'])
    59         if patch.get('id'):
    60             self.browser['patch_id'] = str(patch['id'])
     57        if patch.bug_id():
     58            self.browser["bug_id"] = str(patch.bug_id())
     59        if patch.id():
     60            self.browser["patch_id"] = str(patch.id())
    6161
    6262    def _add_results_file(self, results_file):
  • trunk/WebKitTools/Scripts/webkitpy/steps/applypatch.py

    r52714 r53133  
    3939
    4040    def run(self, state):
    41         log("Processing patch %s from bug %s." % (state["patch"]["id"], state["patch"]["bug_id"]))
     41        log("Processing patch %s from bug %s." % (state["patch"].id(), state["patch"].bug_id()))
    4242        self._tool.scm().apply_patch(state["patch"], force=self._options.non_interactive)
  • trunk/WebKitTools/Scripts/webkitpy/steps/applypatchwithlocalcommit.py

    r52714 r53133  
    4141        if self._options.local_commit:
    4242            commit_message = self._tool.scm().commit_message_for_this_commit()
    43             self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"]["name"])
     43            self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"].name())
  • trunk/WebKitTools/Scripts/webkitpy/steps/closebug.py

    r52714 r53133  
    4444        # Check to make sure there are no r? or r+ patches on the bug before closing.
    4545        # Assume that r- patches are just previous patches someone forgot to obsolete.
    46         patches = self._tool.bugs.fetch_patches_from_bug(state["patch"]["bug_id"])
     46        patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
    4747        for patch in patches:
    48             review_flag = patch.get("review")
    49             if review_flag == "?" or review_flag == "+":
    50                 log("Not closing bug %s as attachment %s has review=%s.  Assuming there are more patches to land from this bug." % (patch["bug_id"], patch["id"], review_flag))
     48            if patch.review() == "?" or patch.review() == "+":
     49                log("Not closing bug %s as attachment %s has review=%s.  Assuming there are more patches to land from this bug." % (patch.bug_id(), patch.id(), patch.review()))
    5150                return
    52         self._tool.bugs.close_bug_as_fixed(state["patch"]["bug_id"], "All reviewed patches have been landed.  Closing bug.")
     51        self._tool.bugs.close_bug_as_fixed(state["patch"].bug_id(), "All reviewed patches have been landed.  Closing bug.")
  • trunk/WebKitTools/Scripts/webkitpy/steps/closebugforlanddiff.py

    r52714 r53133  
    4242    def run(self, state):
    4343        comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
    44         bug_id = state["patch"]["bug_id"]
     44        bug_id = state.get("bug_id") or state["patch"].bug_id()
     45
    4546        if bug_id:
    4647            log("Updating bug %s" % bug_id)
  • trunk/WebKitTools/Scripts/webkitpy/steps/closepatch.py

    r52714 r53133  
    3434    def run(self, state):
    3535        comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
    36         self._tool.bugs.clear_attachment_flags(state["patch"]["id"], comment_text)
     36        self._tool.bugs.clear_attachment_flags(state["patch"].id(), comment_text)
  • trunk/WebKitTools/Scripts/webkitpy/steps/obsoletepatches.py

    r52714 r53133  
    4444            return
    4545        bug_id = state["bug_id"]
    46         patches = self._tool.bugs.fetch_patches_from_bug(bug_id)
     46        patches = self._tool.bugs.fetch_bug(bug_id).patches()
    4747        if not patches:
    4848            return
    4949        log("Obsoleting %s on bug %s" % (pluralize("old patch", len(patches)), bug_id))
    5050        for patch in patches:
    51             self._tool.bugs.obsolete_attachment(patch["id"])
     51            self._tool.bugs.obsolete_attachment(patch.id())
  • trunk/WebKitTools/Scripts/webkitpy/steps/updatechangelogswithreviewer.py

    r53120 r53133  
    4343
    4444    def _guess_reviewer_from_bug(self, bug_id):
    45         patches = self._tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
     45        patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
    4646        if len(patches) != 1:
    4747            log("%s on bug %s, cannot infer reviewer." % (pluralize("reviewed patch", len(patches)), bug_id))
    4848            return None
    4949        patch = patches[0]
    50         reviewer = patch["reviewer"]
    51         log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (reviewer, patch["id"], bug_id))
    52         return reviewer
     50        log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (patch.reviewer().full_name, patch.id(), bug_id))
     51        return patch.reviewer().full_name
    5352
    5453    def run(self, state):
    55         bug_id = state.get("bug_id") or state["patch"]["bug_id"]
     54        bug_id = state.get("bug_id") or state["patch"].bug_id()
     55
    5656        reviewer = self._options.reviewer
    5757        if not reviewer:
Note: See TracChangeset for help on using the changeset viewer.