murdos / musicbrainz-userscripts

Collection of userscripts for MusicBrainz, by various authors
535 stars 89 forks source link

Add Make Votable checkbox and Edit Note text area #535

Closed jesus2099 closed 1 year ago

jesus2099 commented 1 year ago

Also appends a userscript signature to the Edit Note. Also cleanup metadata block.

This would fix #189.

jesus2099 commented 1 year ago

Currently being tested and several fixes are planned.

jesus2099 commented 1 year ago

I have changed all requested stuff (forum posts by @Tiske_Tisja, @UltimateRiff and @IvanDobsky and GitHub reviews by @kellnerd). :)

jesus2099 commented 1 year ago

I guess I will squash the commits in one commit, at the end.

jesus2099 commented 1 year ago

Maybe I can solve conflict (and squash some commits) on my PC? I will try that, soon.

kellnerd commented 1 year ago

According to the GitHub web editor (hidden behind the "Resolve conflicts" button), the only conflict is the version number.

But before merging, I would prefer if the edit note creation logic was refactored into a function (which accepts the edit mode and returns the ready-to-use edit note string) in order to avoid repeated code and the %edit-mode% placeholder hack. Currently the URL-encoding is happening only for one of the two occurrences of the edit note, for example :innocent:

jesus2099 commented 1 year ago

According to the GitHub web editor (hidden behind the "Resolve conflicts" button), the only conflict is the version number.

If you press Next button, there is a second conflict, at the bottom. 😉

I would prefer if the edit note creation logic was refactored into a function (which accepts the edit mode and returns the ready-to-use edit note string)

Ok, good, I will do that.

Currently the URL-encoding is happening only for one of the two occurrences of the edit note

This is normal, as it should only be done on the GET request (add work), not on the POST request (add relationship).

kellnerd commented 1 year ago

If you press Next button, there is a second conflict, at the bottom. 😉

You're right, I have missed the scrollbar... and the "3 conflicts" label... it's getting late here :sweat_smile:

This is normal, as it should only be done on the GET request (add work), not on the POST request (add relationship).

That's not true for all POST requests, in HTTP there are two ways to POST data: application/x-www-form-urlencoded and multipart/form-data. We're using the first one here, so we have to encode values, as the name says. And if I'm not blind again, we are using POST in both cases? https://github.com/murdos/musicbrainz-userscripts/blob/82866569f3d97156cde2f14523ec22dfe218c148/batch-add-recording-relationships.user.js#L1124

jesus2099 commented 1 year ago

Ah OK, I will check my scripts to see how I usually do POST requests, perhaps as multipart/form-data.

Oh yes, I often did use encodeURIComponent each time necessary with application/x-www-form-urlencoded and sometimes multipart/form-data and often also let the browser do the job by building whole forms and submit them.

Thanks for this reminder!

Good night. I have solved conflicts but I will do the other stuff later. :)

BTW:

https://www.w3.org/TR/html401/interact/forms.html#h-17.13.4

The content type "application/x-www-form-urlencoded" is inefficient for sending large quantities of binary data or text containing non-ASCII characters. The content type "multipart/form-data" should be used for submitting forms that contain files, non-ASCII data, and binary data.

Interesting!

jesus2099 commented 1 year ago

My Linux PC is dead and it seems I cannot install eslint on my office Windows laptop.

BTW, Super! I was working on another repo and found out that it was (without admin rights) super easy to install eslint on the office Windows laptop!

So, when I have time to go on working on this PR, I can use a faster computer that does not crash all the time! :D

jesus2099 commented 1 year ago

@kellnerd, I will make the function and make sure everything is alright about the encoding of parameters but I had doubts as my tests were already OK like that.

I have added encodeURIComponent where we build the data as a string. But I saw no problems where we build the data as an object.

And it's because jQuery.post() is a shortcut to jQuery.ajax(), and when we use a PlainObject for data, jQuery does the necessary, already.

jesus2099 commented 1 year ago

@kellnerd, could you check what you prefer between GUI 1, 2 and 3? I thought separate recording filters looked weird, but maybe I'm wrong.

jesus2099 commented 1 year ago

I will try to limit the amount of commits by using squash, soon. I'll tell you when I think it's finished. :) Thank you very much, I took you too much time!

jesus2099 commented 1 year ago

I have reduced the amount of commits as much as possible, now. I hope I did no mistakes. I did no mistakes.