Closed Bug 1228311 Opened 9 years ago Closed 7 years ago

Remember try syntax from last run on review

Categories

(MozReview Graveyard :: General, defect)

Product:
MozReview Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ato, Assigned: manishearth)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1228311 - Remember last try syntax used for a review;
58 bytes, text/x-review-board-request
smacleod
: review+
Details 
It would be convenient if the “Trigger a try run” dialogue remembered the last try syntax used on the review.

A try syntax like the one I use for Marionette,

-b do -e -p linux64,linux64-mulet,macosx64,win64,linux64_gecko -u marionette[x64,Mulet Linux,10.6,Windows 8,b2g],marionette-e10s[x64,Mulet Linux,10.6,Windows 8,b2g],luciddream[x64,Mulet Linux,10.6,Windows 8,b2g],web-platform-tests[x64,Mulet Linux,10.6,Windows 8,b2g],marionette-webapi[x64,Mulet Linux,10.6,Windows 8,b2g],gaia-unit[x64,Mulet Linux,10.6,Windows 8,b2g],gaia-js-integration[x64,Mulet Linux,10.6,Windows 8,b2g] -t none

is virtually impossible to remember by heart.
Component: Treeherder → MozReview
Product: Tree Management → Developer Services
Version: --- → unspecified
Product: Developer Services → MozReview
I was about to file a similar issue. I also use a rather complex try syntax and it'd be really awesome if mozreview would remember it for me (maybe storing it in local storage would be a good start).

Andreas mentioned that this would be convenient on a single review, but I would like mozreview to *always* remember my last try syntax, even on new reviews. I only work on devtools, and tend to use the same try syntax over and over again.
Working on this, though bug 1173842 is also touching this code so I need to wait for it to happen
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Depends on: 1173842
Attached patch Patch (obsolete) — — Splinter Review 
This is the patch I've got so far. It builds on the patch in bug 1173842.

Feedback on the general approach? I'll make this a proper patch and use reviewboard when the other bug lands.
Attachment #8782326 - Flags: feedback?(dwalsh)
Attachment #8782326 - Attachment is obsolete: true
Attachment #8782326 - Flags: feedback?(dwalsh)
The new patch fixes this.

Given that bug 1173842 adds another way that the trychooser input can be auto-set (and auto-cleared), this tweaks that behavior. Now, on page load, the trychooser iframe isn't allowed to reset the try syntax input's value with a blank value unless there was no old try syntax (in which case it's not stomping on any feet), or the input has changed since the old try syntax was filled.

The trychooser iframe is still allowed to set the value to something other than blank.
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review83978

Ah, sorry this review took so long! Looks good, just a couple of nits if you have the time to update things - if not let me know and we'll have someone on the team finish things.

::: pylib/mozreview/mozreview/fields.py:132
(Diff revision 2)
> +            try:
> +                try_syntax = request.extra_data.get('try_syntax')
> +            except IndexError:
> +                pass

Since you're using `get()` you don't need the try block. `get()` will return a default value of `None` if they key isn't found.

So I'd think with the current code you'd end up with  `<div id="ci-actions" data-try-syntax="None">` when there is no previous try syntax. Is that not what you were seeing?

You can either use `{{try_syntax|default_if_none:""}}` to convert the python `None` in the template to the empty string, or do `try_syntax = request.extra_data.get('try_syntax', '')` to default the value to an empty string when the key isn't found.

::: pylib/mozreview/mozreview/static/mozreview/js/autoland.js:89
(Diff revision 2)
> +      if (oldTrySyntax !== undefined) {
> +        box.find('#mozreview-autoland-try-syntax').val(oldTrySyntax);
> +      }
> +
> +      box.find('#mozreview-autoland-try-syntax').on('change', function () {allowReset = true});

I think I'd prefer we move these up to where the old try syntax is grabbed, rather than after the listener
Attachment #8787883 - Flags: review?(smacleod) → review-
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review83978

> Since you're using `get()` you don't need the try block. `get()` will return a default value of `None` if they key isn't found.
> 
> So I'd think with the current code you'd end up with  `<div id="ci-actions" data-try-syntax="None">` when there is no previous try syntax. Is that not what you were seeing?
> 
> You can either use `{{try_syntax|default_if_none:""}}` to convert the python `None` in the template to the empty string, or do `try_syntax = request.extra_data.get('try_syntax', '')` to default the value to an empty string when the key isn't found.

This is within a loop; I want to set it to the last available try syntax, so I need the try-catch. I'll use indexing instead of get though.
Updated. Haven't been able to test it again, would be great if you could.
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review83978

> This is within a loop; I want to set it to the last available try syntax, so I need the try-catch. I'll use indexing instead of get though.

This loop is going from most recent, to least recent autoland request and these lines will only be excuted once for each repository_url. This means only a single iteration will actually have `'try_syntax'` in `extra_data`. If we want to be a little more defensive we should go with:
```
try_syntax = try_syntax or request.extra_data.get('try_syntax', '')
```

I'm going to reopen this issue.
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review84186

Clearing review flag, see https://reviewboard.mozilla.org/r/76494/#comment108200 reopened.
Attachment #8787883 - Flags: review?(smacleod) → review-
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review83978

> This loop is going from most recent, to least recent autoland request and these lines will only be excuted once for each repository_url. This means only a single iteration will actually have `'try_syntax'` in `extra_data`. If we want to be a little more defensive we should go with:
> ```
> try_syntax = try_syntax or request.extra_data.get('try_syntax', '')
> ```
> 
> I'm going to reopen this issue.

(fixed, forgot to mark as resolved)
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review106436
Attachment #8787883 - Flags: review?(smacleod) → review+
manishearth, is this ready to land?  If so, go ahead & use autoland (or NI smacleod if you don't have perms).
Flags: needinfo?(manishearth)
It needed a rebase. Rebased, but you probably should take a look at it again; I can't test it anymore (don't have the setup).
Flags: needinfo?(manishearth) → needinfo?(smacleod)
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/47b2c9c37e7c
Remember last try syntax used for a review; r=smacleod
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(smacleod)
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review118754

Just making a quick drive-by review because a few things here look a bit confusing to me.

::: pylib/mozreview/mozreview/hooks.py:83
(Diff revision 5)
>              if request.repository_url in repo_urls:
>                  continue
>  
>              repo_urls.add(request.repository_url)
>              latest_autoland_requests.append(request)
> +            try_syntax = try_syntax or request.extra_data.get('try_syntax', '')

Shouldn't this be `try_syntax = request.extra_data.get('try_syntax', '') or try_syntax` so that we update `try_syntax` whenever the request contains it? Or do we want to ignore what's in the `request` once the `try_syntax` has been cached?

::: pylib/mozreview/mozreview/hooks.py:95
(Diff revision 5)
>              'current_child_num': current_child_num,
>              'next_child': next_child,
>              'prev_child': prev_child,
>              'latest_autoland_requests': latest_autoland_requests,
>              'user': user,
> +            'try_syntax': 'try_syntax',

Why is this not `'try_syntax': try_syntax`?
Comment on attachment 8787883 [details]
Bug 1228311 - Remember last try syntax used for a review;

https://reviewboard.mozilla.org/r/76494/#review118754

(FWIW this already landed)

> Shouldn't this be `try_syntax = request.extra_data.get('try_syntax', '') or try_syntax` so that we update `try_syntax` whenever the request contains it? Or do we want to ignore what's in the `request` once the `try_syntax` has been cached?

I think the iteration here is in order of most to least recent, so this keeps the field updated with the most recent try syntax.

> Why is this not `'try_syntax': try_syntax`?

Ack, made a mistake when rebasing. This has been fixed by someone else in master, though.
Hi Manish, 

I'm glad to see this cool feature has already landed! I'd like to know how do we use this new feature to remember last try syntax? Because I don't see last try syntax autofill on "Trigger a try run” dialogue". Do I misunderstand something here?

Thanks!
Flags: needinfo?(manishearth)
Looks like the code that passed try_syntax to the commits template got clobbered in https://hg.mozilla.org/hgcustom/version-control-tools/rev/0aa44008be78 (ctrl-f try_syntax, it was removed but not readded).

I don't really have the time to set up the VM again for this. :imadueme, could you have a look?
Flags: needinfo?(manishearth) → needinfo?(imadueme)
Yup,  something definately went wrong in that patch I made.  
I'll take a look this week, though I will be in New Hire Onboarding fore most of my time. 
Sorry for the regression!
Flags: needinfo?(imadueme)
Hmm. I don't really know what does "commits template got clobbered" mean. Is there something backout due to test failures?
Flags: needinfo?(imadueme)
"Looks like the code (that passed try_syntax (to the commits template)) got clobbered"

There's the try_syntax stuff in https://hg.mozilla.org/hgcustom/version-control-tools/rev/0aa44008be78#l1.69 , which got deleted in that commit, probably across a rebase.
Sorry, you can track the status of the fix at Bug 1357852.
The patch just got r+'d so the fix should be coming soon, it was super simple.
Flags: needinfo?(imadueme)
This should be fixed on production now. Apologies for the wait!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: