renovate the file upload #58

mawinter69 · 2023-06-03T17:33:01Z

instead including the upload functionality in an iframe directly include it in the form. The file input has no name attribute so it will not be up uploaded when submitting.
When clicking the button the file will by uploaded asynchronously and a notification bar is shown to the user with the result.

script and css is included via an adjunct is plain vanilla javascript.

requires #56

New look:

Testing done

Manual testing. Go to Jenkins -> Manage -> Configure
Click on Upload under the Add Link button -> Warning is shown No file chosen.
Click on Choose file and select a file, click Upload -> Notification is shown where the file is found (The notification doesn't allow html) so the path is printed in single quotes. This notification is sticky so it will not disappear.
Click Upload again -> An error is shown that the file already exists.

Enter path to uploaded file in a Link and Save -> See the chosen image in the link.

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

instead including the upload functionailty in an iframe directly include it in the form. The file input has no name attribute so it will not be up uploaded when submitting. When clicking the button the file will by uploaded asynchronously and a notification bar is shown to the user with the result. script and css is included via an adjunct is plain vanilla javascript.

damianszczepanik · 2023-06-06T22:05:28Z

src/main/resources/hudson/plugins/sidebar_link/Messages.properties

-NoFile=No file uploaded.
-DupName=File with this name already exists in userContent.
-Back=Back
+uploaded=Uploaded image file; use {0} in Link Icon field above.


do we need to change it to lowercase ?

checkstyle complained that the methods used in java start with a capital letter

it did not complain so far so I don't see reason why it starts now

It was spotbugs see https://ci.jenkins.io/job/Plugins/job/sidebar-link-plugin/job/PR-58/1/spotbugs/
Probably because I changed the Messages.properties

can you revert those changes - I don't see value of them, just key renaming and loosing translations which were deleted

Done.
What might be done is move the ja translations from the deleted startUpload_ja.properties to the config_ja.properties. The texts are identical except for one where just the traliing : is removed.

damianszczepanik · 2023-06-06T22:06:39Z

src/main/resources/hudson/plugins/sidebar_link/SidebarLinkPlugin/upload.js

@@ -0,0 +1,25 @@
+Behaviour.specify("#sidebar-link-upload-button", "sidebar-link-upload", 0, function(e) {


I'm not happy to merge this JS code, prefer to have ready to use library

What you mean with ready to use library?
Even when you have a library you need to customize calling the library.

I believe this is not the only plugin that uploads the file and I believe there is one common reusable approach for Jenkins like for other controls

Usually files are uploaded during a form submit (e.g. when you have a build with a file parameter and you start a build).
But obviously this is not desired here as you want to upload the file and then be able to configure it without the need to press apply or save. Otherwise the existing approach with the iframe would not be here.
This iframe is not really the way to go in my eyes. After uploading you get a simple html snippet that contains no styling and you need to press back to get back to the place where you can upload files.
Of course you can include the file upload in every submit of the Jenkins config though that seems also not the right approach for me.

@daniel-beck do we have reusable component for file upload instead of having this custom JavaScript code ?

how about some opensource library eg https://github.com/pqina/filepond

Jenkins is using pure field for uploading the pluign

Feel free to use whatever you want in your plugin.

For something provided by core, to be generally usable by plugins, it should probably come in the form of a Jelly taglib to abstract away the implementation and future-proof it. So that'll require a bit of work, and this hasn't really come up before AFAIK.

I think using such a library is overkill. And it will not prevent that you need some javascript to connect the file input field with the library. Also for the upload you need to include a crumb, so in the end I doubt that when using the library you will get less javascript code.
And half of the code that we have now is related to notifying the user of the result.
Having a taglib might be useful, but I'm not aware of any other use case like the one here, where the upload should not be done as part of a form submission using the save or apply buttons.
If you want I can create a jelly taglib here in the plugin.

If you want I can create a jelly taglib here in the plugin.

Sounds like good plan @mawinter69

I also found jenkinsci/jenkins#7452

damianszczepanik · 2023-06-06T22:07:49Z

This PR is conflicted with the master branch

damianszczepanik · 2023-06-07T17:05:49Z

src/main/resources/hudson/plugins/sidebar_link/SidebarLinkPlugin/upload.js

@@ -0,0 +1,25 @@
+Behaviour.specify("#sidebar-link-upload-button", "sidebar-link-upload", 0, function(e) {


I believe this is not the only plugin that uploads the file and I believe there is one common reusable approach for Jenkins like for other controls

mawinter69 · 2023-07-19T21:46:51Z

Any plans to merge this?
On 2.387.3 the UI is broken now, the iframe causes scrolling.

My change would fix this.

…into renovate

jonesbusy · 2023-11-22T05:52:23Z

Any plans to merge this?
On 2.387.3 the UI is broken now, the iframe causes scrolling.

+1

damianszczepanik · 2023-11-22T22:04:59Z

To be honest I don't feel that this change is the perfect way of fixing problem we have:

this js file is hard to maintain
js file has some hard coded not translatable labels
I'm not able to validate custom css which are introduced here, prefer to reuse once that are already delivered by Jenkins
and the most problematic is - is that JS code secure - I'm not able to validate it, you can't prove it

so the best for me is to have this component in Jenkins core and use it here as the reference.

Same problem is for plugin upload - ugly button is presented by Jenkins from a very long time

mawinter69 · 2023-11-22T23:37:29Z

To be honest I don't feel that this change is the perfect way of fixing problem we have:

this js file is hard to maintain

Just because you don't feel comfortable with js doesn't mean it is hard to maintain. The script has a total of less than 30 lines. With a minimal experience in javascript and knowledge about some basic things that Jenkins provides for use in javascript (notificationBar, Behaviour) it is easy to understand.

js file has some hard coded not translatable labels

I fixed that, there was one text that was hard coded, I made it translatable via jelly.

I'm not able to validate custom css which are introduced here, prefer to reuse once that are already delivered by Jenkins

I removed the css

and the most problematic is - is that JS code secure - I'm not able to validate it, you can't prove it

See my note above. With some basic knowledge in javascript one can easily see that the code is uncritical. If you want I can explain you for every line what it is doing so you can assure yourself that the code is secure. You can also ask some of the core maintainers if they are willing to verify if the code is secure. In Core basically all PR with changes in the UI are looked at from a security perspective e.g. if a change will introduce a XSS vulnerability.

so the best for me is to have this component in Jenkins core and use it here as the reference.

The custom folder icon plugin is also doing asynchronous file upload, but it also uses a js library named croppie in that method, that allows to resize/crop images.
So I doubt this will ever become a core functionality.

Same problem is for plugin upload - ugly button is presented by Jenkins from a very long time

The upload of plugins in core is done by a form submit, as it is done for file parameters in jobs and in some other places and plugins. And there is no ugly button in recent Jenkins version.

You will need to decide if you want to keep your plugin broken as it is, accept this PR or find another way to fix it.
The feature to directly upload an image is just a convenience, in environments where you use configuration as code (be it via CasC plugin or some other way) you don't need it normally you would ensure that the required images are available. And as an admin you can also just write a simple freestyle job that runs on the controller and uploads a file to the userContent directory.
So just completely removing the file upload would also be a valid option in my eyes, definitely better than doing nothing and have a broken UI.

mawinter69 added 2 commits June 3, 2023 19:22

fix spotbuts warnings

9d0344f

damianszczepanik reviewed Jun 6, 2023

View reviewed changes

Merge branch 'master' into renovate

02ffb63

damianszczepanik requested changes Jun 7, 2023

View reviewed changes

mawinter69 added 4 commits July 22, 2023 12:44

ensure translations are working

21c3ae6

Merge branch 'renovate' of github.com:mawinter69/sidebar-link-plugin …

3dfcb3b

…into renovate

revert

e497932

as taglib

42feb78

mawinter69 force-pushed the renovate branch from 4d85a94 to 42feb78 Compare October 15, 2023 11:47

remove css, make text translatable

f00dc94

mawinter69 closed this Mar 9, 2024

mawinter69 deleted the renovate branch March 9, 2024 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

renovate the file upload #58

renovate the file upload #58

mawinter69 commented Jun 3, 2023 •

edited

Loading

damianszczepanik Jun 6, 2023

mawinter69 Jun 7, 2023

damianszczepanik Jun 7, 2023

mawinter69 Jun 7, 2023

damianszczepanik Jul 22, 2023

mawinter69 Jul 22, 2023 •

edited

Loading

damianszczepanik Jun 6, 2023

mawinter69 Jun 7, 2023

damianszczepanik Jun 7, 2023

mawinter69 Jun 7, 2023

damianszczepanik Jul 23, 2023

damianszczepanik Jul 24, 2023

damianszczepanik Jul 24, 2023

daniel-beck Jul 25, 2023

mawinter69 Jul 25, 2023

damianszczepanik Oct 14, 2023

damianszczepanik commented Jun 6, 2023

damianszczepanik Jun 7, 2023

mawinter69 commented Jul 19, 2023

jonesbusy commented Nov 22, 2023

damianszczepanik commented Nov 22, 2023

mawinter69 commented Nov 22, 2023

		@@ -0,0 +1,25 @@
		Behaviour.specify("#sidebar-link-upload-button", "sidebar-link-upload", 0, function(e) {

renovate the file upload #58

renovate the file upload #58

Conversation

mawinter69 commented Jun 3, 2023 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mawinter69 Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianszczepanik commented Jun 6, 2023

Choose a reason for hiding this comment

mawinter69 commented Jul 19, 2023

jonesbusy commented Nov 22, 2023

damianszczepanik commented Nov 22, 2023

mawinter69 commented Nov 22, 2023

mawinter69 commented Jun 3, 2023 •

edited

Loading

mawinter69 Jul 22, 2023 •

edited

Loading