1066727 - [Job exclusions] Changes to job exclusions lists don't take effect until the visibility profile is also re-saved

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

If I change an exclusion already included in a visibility profile, the changes don't take effect when I save the changes. I instead either have to reload the page or open and save the visibility profile (which forces a refresh). It would be nice if the change went into effect immediately.

Comment 1

[Ed Morley [:emorley]]

I hit this just now. Even after force refreshing the page, the changes made to a job exclusion entry (for bug 1037001) did not take effect until I pressed "change" on the "default" job exclusion profile and pressed save.

Comment 3

[Ed Morley [:emorley]]

There's another variation of the problem, from bug 1120043:

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from bug 1120043 comment #2)
> What I've observed since Cam's changes went into effect about not
> downloading hidden job data by default is that I often have open the filter,
> save it, open the default profile, save it, then force-refresh *per-tree* to
> make them consistently go away.
> 
> And yeah, that last part is really a pain. To make it clear what I'm saying,
> say I did the above on mozilla-central. If I then switch to an inbound tab
> and ctrl+refresh, the jobs are still there. I have to do the above procedure
> on that tree as well for them to consistently go away. Wash, rinse, repeat,
> for every other tree.

Cameron, do you know what might be happening here?

Comment 4

[Cameron Dawson [:camd]]

I DO expect you to have to reload the page, when updating job exclusions or an exclusion profile.  We could try to automatically do that.  Though that might be tough on tabs other than the one you're on.  Perhaps we should have a ``last_modified`` field on the profile and if that's newer than the last time you fetched the first resultset, we reload the page.  Something along those lines.

In comment 1, I could see how that would happen.  There may be a bug there where changing the job exclusions doesn't trigger a re-save (which re-figures the excluded job signatures) on the profile.  That re-save on the profile is critical.  Then a page reload to fetch the resultsets with the updated exclusions.

But I'm kind very surprised by Ryan's description in comment 3.  I can't quite imagine how it would require you to re-save the profile on each tab.  The database has already been altered.  So we'll (I'll) need to do 
more digging there as to why and how that can be happening.

Comment 5

[Cameron Dawson [:camd]]

So there are a few potential parts to this:

1. fix having to save the job_exclusion AND the exclusion_profile to make it "take"
2. have a notification to people that the profile was updated
3. if someone has 30 pushes loaded, the reload will get them back to default 10.  Perhaps we should persist count to what has been loaded in the URL.

Comment 6

[Cameron Dawson [:camd]]

Simple fix to update the ``flat_exclusions`` on save of a ``job_exclusion``.  In fact, the save of the ``job_exclusion`` was already re-saving the ``exclusion profile``, but that save wasn't explicitly updating its ``flat_exclusions``.  The ``flat_exclusions`` are the important part of which jobs to exclude for a profile.  Now any save on the exclusion profile updates its ``flat_exclusion``.

Comment 7

[Cameron Dawson [:camd]]

Attachment: service PR

Comment 8

[Cameron Dawson [:camd]]

Questions for Sheriffs:

1: on this update, would you want it to automatically reload the current tab/page?
2: would you want to detect a change on other tabs and have it automatically reload? (seems undesirable to me as you might be in the middle of something)
3: Alternate to 2: just bring up a sticky notification stating they need to reload?

Comment 9

[Ed Morley [:emorley]]

Comment on attachment 8596614 [details] [review]
service PR

:-D

Comment 10

[Ed Morley [:emorley]]

Morphing the summary of this bug to be about the issue the PR fixes (which is the most pressing). Let's file a new bug for the "auto refresh and notify other clients" part (once we've figured out the answers to comment 8) - since that's more involved (and also something TBPL didn't do either), so would be probably be best in a separate bug :-)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Cameron Dawson [:camd] from comment #8)
> Questions for Sheriffs:
> 
> 1: on this update, would you want it to automatically reload the current
> tab/page?

I'd want it to take effect immediately, yes. I wouldn't want to lose state in the process.

> 2: would you want to detect a change on other tabs and have it automatically
> reload? (seems undesirable to me as you might be in the middle of something)

Ideally yes, but same as #1 RE: not losing state.

> 3: Alternate to 2: just bring up a sticky notification stating they need to
> reload?

Probably not overly important. If I have active tabs up (i.e. I'm working) and someone's messing with visibility on trees we care about, odds are good I'm going to know about it :)

Comment 12

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/c7205463351910f4c63f43f349975502a4a8aa7d
Bug 1066727 - update exclusion profile on save

Comment 13

[Cameron Dawson [:camd]]

Created Bug 1157826 to cover the auto-refresh part of this

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Cameron Dawson [:camd] from comment #8)

> 1: on this update, would you want it to automatically reload the current
> tab/page?
As long is the reload goes back down to the oldest shown revision, sure. Or if there was a way to unload the old visibility profiles and re-load the newly changed ones without a page reload, that'd work too.

Note, I wouldn't want it to automatically change to a tochange/fromchange thing, where newer revisions don't automatically come in. It should keep whatever was already there.

> 2: would you want to detect a change on other tabs and have it automatically
> reload? (seems undesirable to me as you might be in the middle of something)
I'd prefer a notification, especially if a reload loses track of any shown revisions.
> 3: Alternate to 2: just bring up a sticky notification stating they need to
> reload?
I think this would be the best option for other tabs (and especially for other people). If I'm making a change in a tab, I'm probably more okay with that tab getting reloaded when I click "Save"/"Apply". Other tabs in my browser reloading is a bit more unexpected. Having my treeherder tabs randomly reload because someone else changed a profile is totally unexpected.