Launchpad Greasemonkey Scripts

Merge lp://staging/~bryce/launchpad-gm-scripts/stockreplies-revamp into lp://staging/launchpad-gm-scripts

stockreplies-revamp
Merge into master

Proposed by Bryce Harrington on 2020-01-11

Status:	Merged
Merged at revision:	188
Proposed branch:	lp://staging/~bryce/launchpad-gm-scripts/stockreplies-revamp
Merge into:	lp://staging/launchpad-gm-scripts
Diff against target:	930 lines (+268/-626) 2 files modified README (+3/-4) lp_stockreplies.user.js (+265/-622)
To merge this branch:	bzr merge lp://staging/~bryce/launchpad-gm-scripts/stockreplies-revamp
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Brian Murray (community)		2020-01-11	Approve on 2020-01-20
Review via email: mp+377470@code.staging.launchpad.net

Description of the change

This is a rewrite of the stockreplies script.

* The core functionality all works - stock reply data is loaded from external files, and presented to the user as clickable links. On activation, the links fill in the bug report text area, and sets other form widgets like assignee, importance, package, etc.

* Modern Greasemonky drops support for dynamically loading external XML files, but has a reliable mechanism for loading JSON files. So, with this change, stockreply data is stored as JSON rather than XML.

* The previous script implementation had a live-edit functionality. In addition to no longer functioning, this involved a large amount of UI code making it difficult to debug. To simplify script maintenance, this feature has been dropped.

* Due to the above two changes there is no longer a 'reload' functionality. I'd like to have this, but haven't figured out a way to implement it.

I've taken the opportunity to cleanup and restructure the code for the script, which I hope makes it a bit easier to maintain going forward.

Revision history for this message

Brian Murray (brian-murray) wrote on 2020-01-15:

Thanks for working on this, I'm excited to have it functioning again. One thing I seem to recall (but I could be wrong) about the previous one was that you could have different sets of replies for different packages. This ended up reducing the number of replies you had to browse through when trying to find the right one. Does that sound familiar to you and if so does your script do that?

When testing this with a bug report without a package the description ended up containing the following oddity:

"report this bug, LP #1857789 against 1857789"

It looks like PKGNAME had the same value as BUGNUMBER.

I also noticed that importance was not being unset if there was no importance defined in the json file e.g. clicking "good" then "missing". Rather than setting it to "Undecided" if importance is null it should probably set it to whatever the importance originally was to prevent mistakenly overwriting data. That same thing also appears to be true when the package is not defined in the json file.

Otherwise this look great thanks again for working on it.

review: Needs Fixing

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-01-15:

Per-package replies would be nice - I filed #845203 way back when to ask for something like that. I don't think I've seen this functionality, could have been done in a fork?

In any case, I did give some thought to this while doing this reimplementation. An approach might be to have multiple .json files each with their own sets of responses for different packages, or even different triaging contexts. Then have the script load several of these .json files and toggle them on or off either by user selection or detection by patch name or other condition. But I decided to just stay focused on getting the basics back in service, and leave that idea for future follow-on work, if enough other people end up using this.

The issue with things not being set back to the original value is an existing bug in the old script, LP: #334040. I was also going to leave this as follow on work, but I can take a look.

And yeah, the script's logic doesn't get the pkgname when there isn't a source package. Probably similar corner cases for undefined users and so on. The logic I'm using for pkgname is lifted directly from the old script so it probably had the same issue. I can also take a look at this.

Revision history for this message

Brian Murray (brian-murray) wrote on 2020-01-16:

Looking again its lp_buttontags.user.js which selectively displays tags for different packages and projects. Sorry for the confusion!

lp://staging/~bryce/launchpad-gm-scripts/stockreplies-revamp updated on 2020-01-18

186. By Bryce Harrington on 2020-01-18

Only set PKGNAME for bug tasks identifying a source package

187. By Bryce Harrington on 2020-01-18

If there is no value to substitute, leave key for user to manually fill in

This isn't great, but it's better than displaying 'null' in the comment.
At least the all-caps keyword should be noticeable.

188. By Bryce Harrington on 2020-01-18

Restore original settings when selecting a response with undefined values

If a given stock reply has an empty or missing value for 'package' the
script would leave the package widget untouched. If a previously
selected stock reply had inserted a value for package, this would cause
the new stock reply to show that changed value - requiring the user to
have to notice and fix it up manually. Instead, keep track of what the
original package was, and restore to that value when it's undefined in
the stock reply.

Follow this same behavior pattern for importance, status, assignee, and
comment.

Fixes: https://bugs.launchpad.net/launchpad-gm-scripts/+bug/334040

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-01-18:

Brian, I think you'll find this fixes the package assignment a lot cleaner now, please have another look when you get a chance.

Try it also on packages with no source package, it should work a bit more sensibly now.

Revision history for this message

Brian Murray (brian-murray) wrote on 2020-01-20:

I've tested the latest version of the script and it seems to address all the concerns I had. Thanks!

review: Approve

Revision history for this message

Bryce Harrington (bryce) wrote on 2020-01-21: