Fix: build-cards not updating automatically without page refresh

[shlomomdahan]

This issue was discovered when working to improve CSP compatibility in the plugin. When working on #150, I noticed that downstream build-cards were not being updated automatically unless I manually refreshed the page.

Problem

• A while back [JENKINS-31746] Improve the layout #95 was opened to improve the pipeline layout, but a regression was introduced as you see below.
• The original implementation used id="pipelines" to target the element, but this was changed to class="pipelines" in PR [JENKINS-31746] Improve the layout #95.
• This caused build-cards to remain in their latest status until the user manually refreshes the page as the triggers were not being caught and processed.
• The correct functionality would be for the user to click "run" and for the cards on the pipeline stream to update as they build and complete.
• However, for this to work, we need to remove a stale check for build status in the line

       <j:if test="${build.getStatus() == 'PENDING'}">

• This is another issue (this script is extracted and removed in [JENKINS-74806] Extract inline script bpp.jelly BuildPipelineView/bpp.jelly  #150 but I am adding for consistency). The problem here is that build.getStatus() is being used as a static check whose values are not updated unless we manually refresh the page and re-fetch the data. So, when we click rerun on a build card, the triggers are fired and never caught because the user has not refreshed the page and the build status is showing whatever it showed when the page first loaded up. \

##Evaluation (RCA)

• It is not obvious why this regression was introduced. I believe that it was probably because the issue only arises when using the re-run functionality and does not occur with a manual refresh, which pulls fresh data from the server and updates the cards accordingly.
• This wasn’t a breaking regression. If the user manually refreshed the page, it worked fine.

Solution

• Update the pipeline selector to the correct table.pipelines class.
• Remove the stale build.getStatus() == "PENDING" check which prevents the triggers from being caught and processed correctly.
• With this change, the triggers are sent to the correct pipelines class which contains those build cards and specific IDs that catch the triggers.
• Now cards upadate automatically and the user does not have to manually refresh the page to see changes in build status.
• Also added minor change to use on() instead of bind() as bind() is deprecated (https://api.jquery.com/bind/)

Testing done

BEFORE
AFTER

To reproduce the problem:

1. Create two freestyle projects: name them upstream and downstream.
   2.In the upstream project, execute a shell with sleep 5 and exit 0 and in the post-build steps, have the upstream build the downstream project.
2. In the downstream project, similarly execute a shell with sleep 5 and exit 0.
3. Run the build once and wait for it to complete.
4. Once complete, hit the re-run button on the bottom right corner of the upstream card. You should see the show progress bar and the upstream project complete. As you wait, notice the downstream card is not being updated. If you waited long enough and refresh the page, you should see downstream is already built and completed successfully without showing the progress.

• One concern is that the previously there might have been a single #pipelines element while now there may be many elements with class="pipelines". This is not an issue as even if we display multiple build histories, each of those steams contain specific build IDs relative to that build stream. When the triggers are fired, they are sent to an array of dependencies which include build IDs specific to that stream. Therefore, we may see triggers for all of those pipelines, but when they are caught and processed, they correctly update cards in their scope.

• Below image shows an example of this:

• 

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[basil]

Can you please do some testing with this to make sure it is safe? Originally there was only a single element with id="pipelines" but now there can be multiple elements with class="pipelines". Do all of them have a trigger, or only just some? Are we correctly applying the trigger onto each one? Answers to these types of questions would give me confidence in this change.

[shlomomdahan]

Can you please do some testing with this to make sure it is safe? Originally there was only a single element with id="pipelines" but now there can be multiple elements with class="pipelines". Do all of them have a trigger, or only just some? Are we correctly applying the trigger onto each one? Answers to these types of questions would give me confidence in this change.

Hi @basil

I took some time to investigate this. Here's what I understand and why the change in the PR can be merged without issues.

1. There is no element with id="pipelines" by default, so the events are not being triggered by default.
2. Even if we display multiple builds (history) on the same pipeline view, the trigger function is targeting a specific set of dependency ID's that relate only to that specific line of build history. For example, in the below image, I set the configuration to display 2 builds, and when hitting re-run on the bottom older build, the process runs correctly, only triggering events for those cards in that build line.
   
3. So while an event may fire on all pipeline tables, only builds matching the specific buildId will actually update.

 jQuery("table.pipelines").on(`show-status-${buildId}`, function() {

[basil]

There is no element with id="pipelines" by default, so the events are not being triggered by default.

But there previously was, in a past commit where the HTML and JavaScript were logically consistent. Then at some point in the commit history the layout was changed without updating the JavaScript. So we need to understand why and how the layout was changed in a previous commit, why that introduced an inconsistency by not updating the corresponding JavaScript, and how this PR addresses that inconsistency. (AI will not be of much help in constructing this argument.)

[shlomomdahan]

Sorry - I misunderstood what you were asking.

I found the specific PR that includes the change from id="pipelines" to class="pipelines": #95
Specifically this commit (Dec 7, 2015): e73665b

Going further back, this cfe5712 is the original commit (Feb 29, 2012) that adds the line to get that #pipelines element.

Given that it’s been unchanged since then, it may have just been an oversight in #95. How it was not noticed until then is strange, but I imagine it has to do with the fact that when you manually hit refresh on the browser, fresh data from the server is pulled in and the cards display that information accordingly. It is only when you hit re-run and do nothing that you do not see cards updating.

With this change, if you do hit re-run and wait, eventually the updates propagate to the downstream cards and they update correctly.

[basil]

I am just not seeing a clear argument here. Yes #95 introduced a regression that has gone unnoticed, but how? And how does this PR fix it?

Please read e.g. https://github.com/delphix/.github/blob/master/pull_request_template.md?plain=1 and formulate this PR in this manner, including testing done. Explaining how #95 introduced a regression would go under the "Evaluation" section. Five Whys would be helpful here. The testing section should explain how to reproduce the original problem and how this PR addresses the problem.