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

RESOLVED FIXED

Status

Tree Management
Treeherder
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: RyanVM, Assigned: camd)

Tracking

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
emorley
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
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.
Priority: -- → P2
Priority: P2 → P3
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.
Priority: P3 → P2
Duplicate of this bug: 1120043
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?
Flags: needinfo?(cdawson)
(Assignee)

Comment 4

4 years ago
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.
Flags: needinfo?(cdawson)
Priority: P2 → P3
(Assignee)

Updated

3 years ago
Assignee: nobody → cdawson
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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``.
(Assignee)

Comment 7

3 years ago
Created attachment 8596614 [details] [review]
service PR
Attachment #8596614 - Flags: review?(emorley)
(Assignee)

Comment 8

3 years ago
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?
Flags: needinfo?(wkocher)
(Assignee)

Updated

3 years ago
Flags: needinfo?(ryanvm)
Comment on attachment 8596614 [details] [review]
service PR

:-D
Attachment #8596614 - Flags: review?(emorley) → review+
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 :-)
Priority: P3 → P2
Summary: [Job exclusions] Changes to an existing job exclusion don't take effect immediately → [Job exclusions] Changes to job exclusions lists don't take effect until the visibility profile is also re-saved
(Reporter)

Comment 11

3 years ago
(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 :)
Flags: needinfo?(ryanvm)
(Assignee)

Updated

3 years ago
Blocks: 1157826
(Assignee)

Comment 13

3 years ago
Created Bug 1157826 to cover the auto-refresh part of this
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(wkocher)
You need to log in before you can comment on or make changes to this bug.