Closed Bug 683833 Opened 10 years ago Closed 7 years ago

Clicking the "<< Back" link in the tree admin panel after making changes should trigger a refresh

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: philor, Unassigned)

Details

(Keywords: sheriffing-untriaged)

Attachments

(1 file, 2 obsolete files)

Took me probably ten times before I got out of following this set of steps every single time:

1. Click Tree Info - Open tree admin panel
2. Make some changes, hiding Jetpack is usually a good one
3. Save the changes
4. Click the "<< Back" link
5. See that what you thought you hid is still visible, go back in, try to find where you missed hiding what you wanted, try to figure out what you might have hidden instead, eventually realize that it didn't actually refresh after you finished.

Forcing a refresh on << Back would keep me from being stupid, and I'm pretty sure that's the goal of most software.
Whiteboard: [sheriff-want]
There is something wrong with the patch that I don't understand. I go into the tree admin panel.  Hide stuff.  Save. Click on '<< Back',  I need to "reload page" to have the stuff disappear.  However, if I go into tree admin panel, unhide stuff. Click save. Click on "<< Back".  The items reappear without needing to reload page.  Need some feedback on what I'm doing wrong.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #656323 - Flags: feedback?(mbrubeck)
Comment on attachment 656323 [details] [diff] [review]
Refresh data and tree status when clicking on '<< Back' in the tree Admin panel. (v1)

If I understand correctly, the problem is that refreshData updates each job with the new data from the server.  The data from the server doesn't include hidden jobs at all, so if a job was previously visible and is now hidden, it just won't get updated.

This is similar to the reason we can't add or remove "noignore" without a full reload, which comes into play in bug 748833.

As a workaround, you could use location.reload() to reload the whole app.  The downside is that some current state will be lost (e.g. if you have used the "down arrow" button to load more pushes).

A real solution would be to add some way to clear all the old data and start over with new data from the server, or to add info about hidden builds in the server response so they can be hidden on update.  (This would also help so that other users don't need to reload when builds get hidden.)
Attachment #656323 - Flags: feedback?(mbrubeck) → feedback-
To be clear, this is not a problem with your patch exactly, but an existing bug ("refreshing data does not hide newly-hidden jobs") that could probably be filed and fixed separately.
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Comment on attachment 656323 [details] [diff] [review]
> A real solution would be to add some way to clear all the old data and start
> over with new data from the server


After doing using _reloadHiddenBuilders() from data.js, we should be able to do this by doing something similar to what is done in _updateUnstarredFilter() and _updateMachineFilter():

    var pushes = Object.values(this._data.getPushes());
    for (var i = 0; i < pushes.length; ++i) {
      this.handleUpdatedPush(pushes[i]);
    }

Although we should really break this out to Data.js rather than cargo copy/paste this around.

In Data.js we have _notifyUpdatedPushes() already, so I guess we just need a _notifyAllPushes() that calls _notifyUpdatedPushes(Object.values(this._pushes)) - then we can use this in the two existing instances above, as well as for this bug :-)

> or to add info about hidden builds in
> the server response so they can be hidden on update.  (This would also help
> so that other users don't need to reload when builds get hidden.)

If I'm reading the code correctly, the updates already get pushed out to them on the 2 min reload, thanks to:

  _loadPendingAndRunningBuilds: function Data__loadPendingAndRunningBuilds(loadTracker, updatedPushCallback, infraStatsCallback) {
    var self = this;
    this._reloadHiddenBuilders(loadTracker);
    ["pending", "running"].forEach(function (pendingOrRunning) {
      self._getPendingOrRunningBuilds(pendingOrRunning, loadTracker, function (data) {
        var updatedPushes = {};
s/After doing using/After using/
(In reply to Ed Morley [:edmorley] from comment #4)
> If I'm reading the code correctly, the updates already get pushed out to
> them on the 2 min reload, thanks to:

Oh, but that only affects new/updated pushes; their client has no way of knowing whether to update old pushes that haven't yet been forced to update due to say a filter change.

We should perhaps just make _reloadHiddenBuilders() do a refresh all if currentHiddenBuilders != new HiddenBuilders etc
To clarify:
* We have code that already requests that a list of pushes be updated - so we need to add a function to Data.js that calls this for all pushes.
* We then make _reloadHiddenBuilders() call this new function if the newly requested list of builders differs from that already stored (will need to test this doesn't cause unnecessary reloads on the initial TBPL load).
* At the location in your existing patch, we need to then just call _reloadHiddenBuilders() and nothing else.
* Optionally (best as another bug perhaps): We can then switch the other locations that refresh all pushes, to use our new Data.js method.

Matt, have I missed anything? (/completely talking rubbish?!) :-)
(In reply to Ed Morley [:edmorley] from comment #7)
> Matt, have I missed anything? (/completely talking rubbish?!) :-)

I don't really know the Data and Controller code well enough to say.
Ok, I've spent a few hours trawling through the code a bit more and the approach I suggested in comment 7 isn't complete / I believe there is a better way to do this anyway.

I'm going to sit down over the weekend and have a proper look to see the best way of implementing, such that we can easily re-use for bug 748833 too.
Ok so the way noignore works is:
* For pending/running builds/tests, we filter hidden builders from https://secure.pub.build.mozilla.org/builddata/buildjson/builds-pending.js and https://secure.pub.build.mozilla.org/builddata/buildjson/builds-running.js client-side.
* For builds/tests that have finished, we use getRevisionBuilds.php with a noignore parameter specified - ie hidden results are filtered server-side. 

When updating hidden builders, the former is already handled. Every 2 minutes when controller's refreshData() runs (or for example if we were to trigger it sooner after we've used the edit hidden builders UI), data.js's _reloadHiddenBuilders() gets invoked, so that the most up to date hidden builders list is used to filter the latest download of builds-running.js & builds-pending.js. You can see this working by watching the webconsole for getHiddenBuilderNames.php entries every two minutes & by hiding/unhiding things that are pending/running. 

The finished builds case isn't currently handled. On the two minute refresh (note: to make testing faster, you can edit Config.js's loadInterval value to something like 10 seconds), we actually do already re-request getRevisionBuilds.php on every visible push, via data.js's _loadTinderboxDataForPushes() - so that we know when running jobs have finished and we can display them as such. This means we've already got the newly revised list of jobs after a hidden builders list change - we just aren't showing it since _loadTinderboxDataForPushes() only calls _notifyUpdatedPushes() on any pushes where a job has recently finished (which it gets from _addFinishedResultsToPushes()). 

To prove this, you can change the _loadTinderboxDataForPushes() line from:
  self._notifyUpdatedPushes(updatedPushes, updatedPushCallback);
to
  self._notifyUpdatedPushes(Object.values(self._pushes), updatedPushCallback);
...and after editing the hidden builders list, the jobs now update on the 2 minute refresh as expected.

Obviously this is inefficient, since we only want to force updated all pushes, when the hidden builders list has changed. 

At the moment _reloadHiddenBuilders() is only called from inside _loadPendingAndRunningBuilds(), which doesn't really help us, since by then we've already performed _loadTinderboxDataForPushes(). We could just force update all pushes again, but that's a bit of a waste, seeing as loadTinderboxDataForPushes() will have already updated a proportion of them. So we'd be best off doing _reloadHiddenBuilders() earlier.

Therefore, the rough way I see this working:
* Move the reloadHiddenBuilders() call to before _loadPendingAndRunningBuilds() in loadPushRange() and also add it to the line before _loadTinderboxDataForPushes() in refresh(), so it gets called sooner.
* Make _reloadHiddenBuilders() check whether the new hidden builders list matches the old list. If not, lets call the existing _afterHiddenBuildersHaveLoaded() but with an extra bool param indicating whether the list has changed. 
* _filterHiddenBuilds() can be adjusted to ignore this new bool, but _loadTinderboxDataForPushes() can now use it to make updatedPushes redundant if the list has changed, and instead call _notifyUpdatedPushes() with the list of all pushes, which will make the UI update correctly.
* Adjust the hidden builders UI to call a refreshData early (on form submit), so we don't have to wait two minutes to see the changes.
* Test test test!

Let me know if any of that isn't clear :-)
Attachment #656323 - Attachment is obsolete: true
Attachment #677390 - Flags: review?(bmo)
Comment on attachment 677390 [details] [diff] [review]
Refresh when clicking on back link.  Part 1

Review of attachment 677390 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so sorry for taking a few days on this - it's been so long since my initial read through these parts of TBPL in comment 10, that I had to start over. 

I couldn't get this patch to work even after adding to it and trying a few different things (please can you make sure you test patches before asking for review; or if you know they don't work, ask for feedback/help - which I'll be happy to give :-)), so went back to mapping out how the refreshes work again. 

It was then that I noticed that _addFinishedResultsToPushes() didn't do what I thought it did originally (my bad, I went purely by the function name before, which to me read as "add results that have recently finished ie disappeared from pending and running, to the pushes list"). It actually does something more like "fetch all completed+visible jobs on every push and for any that have changed, update the saved pushes array, then return a list of pushes that the UI will need to update".

The suggested plan of attack in comment 10 only deals with the UI, not removing the "completed but now hidden jobs" from the pushes array. In addition, having thought about it more, I believe adding a hasChanged bool to the callback is only making things unnecessarily complex - we can just implement in a similar way to the _hiddenBuildersAreLoading variable.

As such we roughly need to (replaces the list in comment 10):
1) Move the reloadHiddenBuilders() call to before _loadPendingAndRunningBuilds() in loadPushRange() and also add it to the line before _loadTinderboxDataForPushes() in refresh(), so it gets called sooner.
2) Make _reloadHiddenBuilders() check whether the new hidden builders list matches the old list. If not, set a global _hiddenBuildersChanged or similar to true.
3) Make _loadTinderboxDataForPushes() check _hiddenBuildersChanged just before the _notifyUpdatedPushes() call. If the builders list has changed, we should:
  (a) call out to a _clearHiddenResultsFromTheirPushes() (or similar), which will iterate through all pushes and remove any should-be-hidden jobs & ideally keep track of the list of updated pushes. For inspiration, use _clearOldPendingOrRunningResultsFromTheirPushes() and _filterHiddenBuilds() as a starting point :-)
  (b) Reset _hiddenBuildersChanged to false
4) Adjust the hidden builders UI to call a refreshData early (on form submit), so we don't have to wait two minutes to see the changes.
5) Test test test! (don't forget these changes can be really easily tested without using Vagrant; just run index.html from the local filesystem)

Any questions just let me know :-)

::: js/Data.js
@@ +53,5 @@
>      pushlogRequestParams.forEach(function (params) {
>        self._loadPushlogData(params, loadTracker, updatedPushCallback, initialPushlogLoadCallback);
>      });
> +    this._reloadHiddenBuilders(loadTracker);
> +

This needs to be one line lower down, ie inside the conditional.

@@ +661,5 @@
>            return;
>          }
>          self._hiddenBuilders = json;
>          if (self._afterLoadedHiddenBuildersCallback) {
> +          if (self._hiddenBuilders != this._oldHiddenBuilders) {

The default equality operator in JavaScript for Objects only yields true when they refer to the same location in memory, so this will always be false.

Given that the string order of the JSON in this case will always be the same, you can get away with using the stringify method here:
http://procbits.com/2012/01/19/comparing-two-javascript-objects/
Attachment #677390 - Flags: review?(bmo)
Whiteboard: [sheriff-want]
Attached patch wip patch v2Splinter Review
Attachment #677390 - Attachment is obsolete: true
Gone back to square one, so this bug will have to have someone else to give it love.  I might retake this bug when I feel I have the brain cells to match the required skill.  Until then, this is available for anyone to take.
Assignee: ewong → nobody
Status: ASSIGNED → NEW
Product: Webtools → Tree Management
Wontfix since TBPL is being replaced by Treeherder.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.