Bug 171121 – obsolete_attachment should not fail when flags do not exist

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Olivier Blin]

./Tools/Scripts/webkit-patch upload 113124 -g HEAD 
Total errors found: 0 in 5 files
Was that diff correct? [Y/n]: 
Please enter password for encrypted keyring: 
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/show_bug.cgi?id=113124&ctype=xml&excludefield=attachmentdata
Obsoleting 1 old patch on bug 113124
Obsoleting attachment: 195692
"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

./Tools/Scripts/webkit-patch upload 113124 -g HEAD 
Total errors found: 0 in 5 files
Was that diff correct? [Y/n]: 
Please enter password for encrypted keyring: 
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/show_bug.cgi?id=113124&ctype=xml&excludefield=attachmentdata
Obsoleting 1 old patch on bug 113124
Obsoleting attachment: 195692
Traceback (most recent call last):
  File "./Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "./Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/tool/steps/obsoletepatches.py", line 54, in run
    self._tool.bugs.obsolete_attachment(patch.id())
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 790, in obsolete_attachment
    self._find_select_element_for_flag('review').value = ("X",)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 735, in _find_select_element_for_flag
    return self.browser.find_control(type='select', nr=0)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_form.py", line 3101, in find_control
    return self._find_control(name, type, kind, id, label, predicate, nr)
  File "/home/sah0146/vc/webkit.org/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_form.py", line 3185, in _find_control
    raise ControlNotFoundError("no control matching "+description)
webkitpy.thirdparty.autoinstalled.mechanize._form.ControlNotFoundError: no control matching type 'select'

[Olivier Blin]

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

[Olivier Blin]

Created attachment 307752 [details]
Patch

[Olivier Blin]

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.

[Alexey Proskuryakov]

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.

[Olivier Blin]

(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.

[Alexey Shvayka]

Created attachment 389555 [details]
Patch

[Jonathan Bedard]

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.

[Alexey Shvayka]

Created attachment 389568 [details]
Patch

[Alexey Shvayka]

(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.

[Jonathan Bedard]

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!

[WebKit Commit Bot]

Comment on attachment 389568 [details]
Patch

Clearing flags on attachment: 389568

Committed r255605: <https://trac.webkit.org/changeset/255605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59128683>