[JENKINS-73948] Extract inline JavaScript from LockableResourcesRootAction/tableQueue/table.jelly

[yaroslavafenkin]

See JENKINS-73948.

Testing done

Took an example pipeline from the plugin page:

echo 'Starting'
lock('whatever') {
  echo 'Do something here that requires unique access to the resource'
  // any other build will wait until the one locking the resource leaves this block
  sleep 300
}
echo 'Finish'

Firing 2 of such pipelines is enough to make the second one queue for a resource.

For the part that shows a warning notification (that was broken before the change) I've modified this line to make the notification appear faster.

https://www.loom.com/share/cecff681e8f3470fbde067c89430fe00?sid=98596835-70b6-4e2e-9561-9733a91ba64c

https://www.loom.com/share/199c7ed9269c4d879782fa3271ad3049?sid=515ece01-6a67-4475-a851-9a978c60f0bc

Submitter checklist

• The Jira / Github issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
  • The changelog generator for plugins uses the pull request title as the changelog entry.
  • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during the upgrade.
• There is automated testing or an explanation that explains why this change has no tests.
• New public functions for internal use only are annotated with @NoExternalUse. In case it is used by non java code the Used by {@code <panel>.jelly} Javadocs are annotated.

• New or substantially changed JavaScript is not defined inline and does not call eval to ease the future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.
• Any localizations are transferred to *.properties files.
• Changes in the interface are documented also as examples.

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There is at least one (1) approval for the pull request and no outstanding requests for change.
• Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
• Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
• Proper changelog labels are set so that the changelog can be generated automatically. See also release-drafter-labels.
• If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
• java code changes are tested by automated test.

[basil]

Could you please give me your opinion about two issues:

1. In [JENKINS-73891] Extract inline JS in ConfigFilesUI/edit.jelly and ConfigFilesUI/show.jelly config-file-provider-plugin#342 we're using an invisible entry rather than display: none. Which is preferable and why? If they are more or less the same, then I don't have a strong preference which one we follow, but I do prefer that this project's PRs be consistent with each other. That way, we are less likely to have strange bugs caused by inconsistent implementation strategies.
2. In [JENKINS-73891] Extract inline JS in ConfigFilesUI/edit.jelly and ConfigFilesUI/show.jelly config-file-provider-plugin#342 we aren't putting a class attribute but instead using const contentTypeElement = document.querySelector("span[data-content-type]"). Similar question as to which is preferable, and if we had to pick a project-wide convention, which would it be?

[yaroslavafenkin]

1. For me everything that's inside the form tag lib semantically belongs to forms. We do not do anything with forms here, we only want something to hold data that we use in scripts. So if we speak only about holding data I'd stick to spans.
2. I think we should use classes and stay away from sometimes overcomplicated selectors. Class selectors are concise understandable by everyone. In case js files live far away from jelly ones having a class name it is trivially easy to find all parts of the equation. With classes we are better protected against potential future markup changes. Someone may introduce new element that may be unintentionally targeted by those fancy selectors. It is less likely that someone will add a very specific class name to their element unless they know what they're doing.

[basil]

So regarding 1), your preference would be to use a span with display: none when there is a form and a span without display: none inside of an invisibleEntry when there is not a form? Or would you want to use a span with display: none with or without a form? Or am I misunderstanding and your preference is for something else entirely?

Regarding 2), I agree with the argument about avoiding fragility.

[yaroslavafenkin]

I wouldn't use invisibleEntry at all for the sake of holding data for JavaScript.

From its doc:

Invisible <f:entry> type. Useful for adding hidden field values.

Would use it that same way entry is used, but when I don't want the field to be visible. E.g. hold a CSRF key in a form that has to be submitted with the form, but we don't want anyone to tamper with it.

[basil]

Thanks!

[basil]

Thanks for the PR!