| Summary: | obsolete_attachment should not fail when flags do not exist | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Olivier Blin <olivier.blin> | ||||||||
| Component: | Tools / Tests | Assignee: | Alexey Shvayka <ashvayka> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, ashvayka, buildbot, commit-queue, dbates, ews-watchlist, glenn, jbedard, lforschler, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | Other | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Olivier Blin
2017-04-21 10:51:24 PDT
Oops, sorry for the incorrect first lines of the first comment, description should start with: "webkit-patch upload" fails in obsolete_attachment when review/commit-queue flags do not exist. This occurred on https://bugs.webkit.org/attachment.cgi?id=195692&action=edit Created attachment 307752 [details]
Patch
Comment on attachment 307752 [details] Patch >- self._find_select_element_for_flag('review').value = ("X",) >- self._find_select_element_for_flag('commit-queue').value = ("X",) >+ try: >+ self._find_select_element_for_flag('review').value = ("X",) >+ self._find_select_element_for_flag('commit-queue').value = ("X",) >+ except: >+ pass Actually, maybe it would be better to have one try/catch per flag, if only one of these select is present on the attachment details page. It's not clear to me when this happens. Is this when the contributor doesn't have the EditBugs permission? Otherwise, I think that these inputs are always present. (In reply to Alexey Proskuryakov from comment #4) > It's not clear to me when this happens. Is this when the contributor doesn't > have the EditBugs permission? Otherwise, I think that these inputs are > always present. Not sure about the exact cause, I have submitted a few patches before, and never got this issue before. This happened on old bug 113124 from 2013, where the patch did not show any flags in the details page. See https://bugs.webkit.org/attachment.cgi?id=195692&action=edit It has not been obsoleted either actually. Created attachment 389555 [details]
Patch
Comment on attachment 389555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389555&action=review > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:-627 > - # FIXME: Should this use self._find_select_element_for_flag? I don't think we should address these FIXMEs in this change, I would prefer that be another patch. Although, the fixes themselves seem fine. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:849 > +Ignore this message if you don't have EditBugs privileges (https://bugs.webkit.org/userprefs.cgi?tab=permissions)""") I would actually prefer this in two separate log statements rather than breaking indentation. Created attachment 389568 [details]
Patch
(In reply to Olivier Blin from comment #3) > Actually, maybe it would be better to have one try/catch per flag, if only > one of these select is present on the attachment details page. To my observation, both <select>s are present and disabled for unprivileged users. I was hoping to avoid try/catch by checking whether "isobsolete" checkbox is present, but no luck: it is there, but hidden. (In reply to Alexey Shvayka from comment #8) > Created attachment 389568 [details] > Patch Remove FIXME-related changes, extract NO_EDIT_BUGS_MESSAGE, and use ValueError. Comment on attachment 389568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389568&action=review > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:915 > + _log.warning(self.NO_EDIT_BUGS_MESSAGE) Good catch! Comment on attachment 389568 [details] Patch Clearing flags on attachment: 389568 Committed r255605: <https://trac.webkit.org/changeset/255605> All reviewed patches have been landed. Closing bug. |