Bug 174477

Created attachment 315961 [details] Reverted the changes to urls.py and bugzilla.py I changed the url in the urls.py back to wekbkit server instead of my local server. I also reverted the changes to bugzilla.py.

Comment on attachment 315961 [details] Reverted the changes to urls.py and bugzilla.py View in context: https://bugs.webkit.org/attachment.cgi?id=315961&action=review > Tools/ChangeLog:7 > + Please describe your changes in the ChangeLog. Please see https://webkit.org/contributing-code/#changelog-files > Tools/QueueStatusServer/handlers/patch.py:50 > + per_queue_statuses = queue_status.get(status.queue_name, []) As I requested couple of times in-person, please avoid this code duplication. It's a simple if else, you should be able to avoid the duplication. > Tools/QueueStatusServer/handlers/statusbubble.py:91 > + message = "\n\nhttps://build.webkit.org/dashboard/" This is completely unrelated change. Please remove this change from this patch and create a separate bug for this change. Make sure you test it as well.

*** Bug 174728 has been marked as a duplicate of this bug. ***

Created attachment 317149 [details] status bubble link to individual EWS page.

Attachment 317149 [details] did not pass style-queue: ERROR: Tools/QueueStatusServer/handlers/patch.py:58: no newline at end of file [pep8/W292] [5] ERROR: Tools/QueueStatusServer/handlers/patch.py:58: [Patch.get] Instance of 'Patch' has no 'response' member [pylint/E1101] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 317149 [details] status bubble link to individual EWS page. View in context: https://bugs.webkit.org/attachment.cgi?id=317149&action=review > Tools/ChangeLog:9 > + to go directly to the Individual EWS page.I also added made changes to the code so that if the number "added made changes" ? > Tools/ChangeLog:10 > + of items in the queue is 0 then have a message that says waiting in the queue processing hasnt started. hasnt => hasn't. 0 => zero > Tools/ChangeLog:11 > + I also added a link to the list of all of the EWS queues at the bottom if there is no or one EWS on the page. "if there is no or one EWS on the page"? Wouldn't that link be always there? > Tools/ChangeLog:15 > + * QueueStatusServer/handlers/statusbubble.py: Please add a small description of each change here as well. > Tools/QueueStatusServer/handlers/patch.py:52 > + queue_status[status.queue_name] = per_queue_statuses Did you see my previous comment: "As I requested couple of times in-person, please avoid this code duplication. It's a simple if else, you should be able to avoid the duplication."

Created attachment 317428 [details] status bubble link to individual EWS page.

The patch looks good except for the code duplication which I requested earlier. You are not able to simplify the if-else?

Created attachment 317447 [details] status bubble link to individual EWS page. changed the if else statement to two if statements to simplify the code.

Code duplication is still there. It shouldn't be difficult to simplify it.

Comment on attachment 317447 [details] status bubble link to individual EWS page. View in context: https://bugs.webkit.org/attachment.cgi?id=317447&action=review > Tools/QueueStatusServer/handlers/patch.py:51 > + queue_status[status.queue_name] = per_queue_statuses I would recommend rewriting L41-L51 as follows: queue_status = {} for status in statuses: bug_id = status.active_bug_id # Should be the same for every status per_queue_statuses = queue_status.get(status.queue_name, []) per_queue_statuses.append(status) queue_status[status.queue_name] = per_queue_statuses # Show a single EWS status instead of all. if queue_name is not None: single_queue_status = {} single_queue_status[queue_name] = queue_statuses.get(status.queue_name, []) queue_statuses = single_queue_status This is optimal because it allows for existing code to continue functioning as it was, and just picks out the individual status if it's requested.

(In reply to Dean Johnson from comment #11) > Comment on attachment 317447 [details] > status bubble link to individual EWS page. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317447&action=review > > > Tools/QueueStatusServer/handlers/patch.py:51 > > + queue_status[status.queue_name] = per_queue_statuses > > I would recommend rewriting L41-L51 as follows: > > queue_status = {} > for status in statuses: > bug_id = status.active_bug_id # Should be the same for every status > per_queue_statuses = queue_status.get(status.queue_name, []) > per_queue_statuses.append(status) > queue_status[status.queue_name] = per_queue_statuses > > # Show a single EWS status instead of all. > if queue_name is not None: > single_queue_status = {} > single_queue_status[queue_name] = queue_statuses.get(status.queue_name, > []) Oops, this line should read: single_queue_status[queue_name] = queue_statuses.get(queue_name, []) > queue_statuses = single_queue_status > > This is optimal because it allows for existing code to continue functioning > as it was, and just picks out the individual status if it's requested.

Created attachment 317460 [details] status bubble link to individual EWS page.

Created attachment 317581 [details] Updated Patch

Patch looks good. Please do svn update and re-create the patch, to ensure it doesn't fail to apply to repository.

Created attachment 317646 [details] Updated Patch

Created attachment 317647 [details] Updated Patch

Looks good to me.

Comment on attachment 317647 [details] Updated Patch Clearing flags on attachment: 317647 Committed r220434: <http://trac.webkit.org/changeset/220434>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33790314>

Updated the server. These changes are now live.