Skip to content

Commit

Permalink
[FIXED JENKINS-30569] HistoryWidget: fix JS syntax error
Browse files Browse the repository at this point in the history
  • Loading branch information
@csimons
csimons committed Oct 6, 2015
1 parent 5b4b66d commit 6cadf70
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions core/src/main/resources/hudson/widgets/HistoryWidget/index.jelly
View file
Expand Up @@ -101,7 +101,9 @@ THE SOFTWARE.
</tr>
</j:if>
</l:pane>
<script defer="true">
updateBuildHistory("${it.baseUrl}/buildHistory/ajax",${it.nextBuildNumberToFetch});
</script>
<j:if test="!empty(it.nextBuildNumberToFetch)">
<script defer="true">
updateBuildHistory("${it.baseUrl}/buildHistory/ajax", ${it.nextBuildNumberToFetch});
</script>
</j:if>
</j:jelly>

5 comments on commit 6cadf70

@tfennelly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csimons Hi Christopher. Can you explain this change to me please. As I see it, this is disabling the build history refresh i.e. new builds do not appear in the build history until after a full page reload.

@csimons
Copy link
Member Author

@csimons csimons commented on 6cadf70 Oct 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tfennelly Hi Tom. At the time of committing this change, this call to updateBuildHistory() was failing due to a syntax error and so was not completing anyway, so this change does not affect the fact that the updateBuildHistory() call does not succeed in this case. From my understanding, the condition of this block is met only if the build history is collapsed and no build history is being shown, in which case the build history list is hidden and thus refreshing it isn't needed.

@tfennelly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csimons Hi again Christopher. So I'm working on #1641 and after merging upstream, what I see is that the build history doesn't refresh at all when that condition is wrapping it. I'll have a closer look and see if I can reproduce the error from JENKINS-30569. I'm sure you won't mind if I tweak your original fix a bit if I find that what I'm saying above is correct, right?

@tfennelly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csimons Hey Christopher. So it wasn't working because of the changes I had made in #1641. I've changed how the refresh is being disabled, doing it in the JavaScript now instead of in Jelly and it works fine. See tfennelly@01b4ac7.

You okay with this?

@csimons
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, sounds good to me. 👍

Please sign in to comment.