Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-69658] Move internal script block in "failed-test.jelly" to external file #439

Merged
merged 5 commits into from
Oct 3, 2022

Conversation

Jagrutiti
Copy link
Contributor

@Jagrutiti Jagrutiti commented Oct 1, 2022

  • 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
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

What

Moved the following the <script> block that was causing the issue to an external file:

 <script type="text/javascript">
      <!-- TODO make sure load doesn't happen every time -->
      function showFailureSummary(id,query) {
        var element = document.getElementById(id)
        element.style.display = "";
        document.getElementById(id + "-showlink").style.display = "none";
        document.getElementById(id + "-hidelink").style.display = "";

        if (typeof query !== 'undefined') {
          var rqo = new XMLHttpRequest();
          rqo.open('GET', query, true);
          rqo.onreadystatechange = function() { element.innerHTML = rqo.responseText; }
          rqo.send(null);
        }
      }

      function hideFailureSummary(id) {
        document.getElementById(id).style.display = "none";
        document.getElementById(id + "-showlink").style.display = "";
        document.getElementById(id + "-hidelink").style.display = "none";
      }
  </script>

Closes

https://issues.jenkins.io/browse/JENKINS-69658

Sorry, something went wrong.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

the test failures show this isn't working

@Jagrutiti
Copy link
Contributor Author

The error says: ReferenceError: "showFailureSummary" is not defined.

There is still a path issue. I will read about Jelly, add a path in Jelly, and try again.

@Jagrutiti
Copy link
Contributor Author

Hey, @uhafner @timja

This tutorial here:
https://wiki.eclipse.org/Understanding_Jelly_Tags
https://wiki.jenkins.io/display/JENKINS/Basic+guide+to+Jelly+usage+in+Jenkins

Do not explain st: adjunct; I cannot find other tutorials explaining the tag. And how to import files in Jelly.

I did find the tutorial explaining adjunct: https://www.jenkins.io/doc/developer/views/exposing-bundled-resources/#adjuncts

I am not able to figure out where my file path is wrong. Or is it something else?

@timja
Copy link
Member

timja commented Oct 2, 2022

I am not able to figure out where my file path is wrong. Or is it something else?

best to search the jenkinsci GitHub org to see how it's used, lots of this stuff is under-documented so code is the best example

@Jagrutiti
Copy link
Contributor Author

All the checks have passed.

@uhafner was right about this:

I think you need to use the path suffix that starts right after the folder src/main/resources

I was making the mistake of adding the .js extension to the end of the file.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. The 1:1 conversions looks good.

(I wonder if we should also change var to const and the TODO does not make sense to me as well?)

@uhafner uhafner changed the title fix: fixed un-inlining failed-test.jelly [JENKINS-69658] Move internal script block to external file Oct 2, 2022
@uhafner uhafner changed the title [JENKINS-69658] Move internal script block to external file [JENKINS-69658] Move internal script block in "failed-test.jelly" to external file Oct 2, 2022
@Jagrutiti
Copy link
Contributor Author

Jagrutiti commented Oct 2, 2022

About changing var to const

The properties are being reassigned in two places.

function showFailureSummary(id,query) {
        var element = document.getElementById(id)
        element.style.display = "";                           // property changes here
        document.getElementById(id + "-showlink").style.display = "none";
        document.getElementById(id + "-hidelink").style.display = "";

        if (typeof query !== 'undefined') {
          var rqo = new XMLHttpRequest();
          rqo.open('GET', query, true);
          rqo.onreadystatechange = function() { element.innerHTML = rqo.responseText; } // property changes here
          rqo.send(null);
        }
      }

In that case should we change it to const?

But the value is not being changed directly so maybe we can.

@timja
Copy link
Member

timja commented Oct 2, 2022

The properties are being reassigned in two places.

That's not reassigning the value that's modifying properties which is allowed when it's a const

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks!

@timja timja added enhancement and removed internal labels Oct 3, 2022
@timja timja enabled auto-merge (squash) October 3, 2022 08:02
@timja timja merged commit 5c28483 into jenkinsci:master Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants