Bug 36374 - Some of our JS files should stay tidy
Summary: Some of our JS files should stay tidy
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 29002 35616
Blocks: 35813 36120 36373
  Show dependency treegraph
 
Reported: 2024-03-20 15:23 UTC by Martin Renvoize
Modified: 2024-07-03 09:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 36374: Tidy place_booking_modal.js according to our prettier config (44.81 KB, patch)
2024-03-20 15:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Move modal js under /modals (6.05 KB, patch)
2024-03-20 15:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Tidy place_booking_modal.js according to our prettier config (37.46 KB, patch)
2024-03-20 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Move modal js under /modals (6.02 KB, patch)
2024-03-20 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Ensure js/modals/*.js files are tidy (1.78 KB, patch)
2024-03-20 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: flag JS files to keep tidy (2.51 KB, patch)
2024-03-21 07:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36374: flag JS files to keep tidy (2.52 KB, patch)
2024-03-21 07:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36374: Keep tidy files from 'fetch' directory (11.37 KB, patch)
2024-03-21 08:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36374: Tidy place_booking_modal.js according to our prettier config (37.46 KB, patch)
2024-03-21 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Move modal js under /modals (6.02 KB, patch)
2024-03-21 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Ensure js/modals/*.js files are tidy (1.78 KB, patch)
2024-03-21 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: flag JS files to keep tidy (2.58 KB, patch)
2024-03-21 09:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Keep tidy files from 'fetch' directory (11.43 KB, patch)
2024-03-21 09:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Also tidy ticket and concern modals js (14.79 KB, patch)
2024-03-21 09:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Tidy place_booking_modal.js according to our prettier config (37.59 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Move modal js under /modals (6.15 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Ensure js/modals/*.js files are tidy (1.91 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: flag JS files to keep tidy (2.71 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Keep tidy files from 'fetch' directory (11.56 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Also tidy ticket and concern modals js (14.91 KB, patch)
2024-04-29 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Tidy place_booking_modal.js according to our prettier config (37.59 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Move modal js under /modals (6.15 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Ensure js/modals/*.js files are tidy (1.91 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: flag JS files to keep tidy (3.07 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Keep tidy files from 'fetch' directory (11.56 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36374: Also tidy ticket and concern modals js (14.99 KB, patch)
2024-04-29 11:55 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2024-03-20 15:23:40 UTC
For some reason my editor didn't seem to be tidying for JS when I wrote the bookings modal code... as this is still a pretty new area, I think we should run a tidy over it to get it consistent now before we continue working on it.
Comment 1 Jonathan Druart 2024-03-20 15:30:14 UTC
Not reported yet, but I have a list of JS files I would like to keep tidied as well (under fetch dir). We need a QA tests + unit test + git hook to deal with that. Like we do for vue files.

We will need to have an explicit list however as more of our JS files are not tidied.
Comment 2 Martin Renvoize 2024-03-20 15:32:28 UTC
Created attachment 163555 [details] [review]
Bug 36374: Tidy place_booking_modal.js according to our prettier config

This was somehow missed when the file was first added.. my mistake.
Comment 3 Martin Renvoize 2024-03-20 15:32:30 UTC
Created attachment 163556 [details] [review]
Bug 36374: Move modal js under /modals

I also didn't stick to the more recent switch to putting modal js under
a /modals dirctory.
Comment 4 Martin Renvoize 2024-03-20 15:59:54 UTC
Jonathan highlighted to me that my tidy config isn't the same as their using in Vue.. so I'll post a new patch once I've got my configs working as expected
Comment 5 Martin Renvoize 2024-03-20 16:25:56 UTC
Created attachment 163558 [details] [review]
Bug 36374: Tidy place_booking_modal.js according to our prettier config

This was somehow missed when the file was first added.. my mistake.
Comment 6 Martin Renvoize 2024-03-20 16:25:59 UTC
Created attachment 163559 [details] [review]
Bug 36374: Move modal js under /modals

I also didn't stick to the more recent switch to putting modal js under
a /modals dirctory.
Comment 7 Martin Renvoize 2024-03-20 16:26:01 UTC
Created attachment 163560 [details] [review]
Bug 36374: Ensure js/modals/*.js files are tidy
Comment 8 Martin Renvoize 2024-03-20 16:26:58 UTC
/modals is a good enough place to start.. I'll post follow-up bugs to tidy the other two files in there as I know both are basically maintained by me at the moment and not touched by anyone else so far.
Comment 9 Jonathan Druart 2024-03-20 16:57:37 UTC
As we also need other files (koha-tmpl/intranet-tmpl/prog/js/fetch/ should all be tidied), maybe we should have a flag in the files (/* keep me tidy */) we could git grep to list them? For discussion, I can have a look tomorrow. I am a bit afraid to have a hardcoded list in 3 different places (koha, qa, git hook)
Comment 10 Jonathan Druart 2024-03-21 07:45:28 UTC
Created attachment 163587 [details] [review]
Bug 36374: flag JS files to keep tidy
Comment 11 Jonathan Druart 2024-03-21 07:47:50 UTC
Created attachment 163588 [details] [review]
Bug 36374: flag JS files to keep tidy
Comment 12 Jonathan Druart 2024-03-21 08:08:09 UTC
Created attachment 163589 [details] [review]
Bug 36374: Keep tidy files from 'fetch' directory

And tidy them
Comment 13 Jonathan Druart 2024-03-21 08:09:17 UTC
It may be more convenient to add several paths (like here 'modals' and 'fetch'), but then we will need to adjust the 3 different paths, and this is something we should avoid I think.

Not ideal but it's good enough for now for a first step.
Comment 14 Jonathan Druart 2024-03-21 08:18:52 UTC
See also:

git hook changes:
https://gitlab.com/koha-community/koha-testing-docker/-/issues/414

qa script changes:
https://gitlab.com/koha-community/qa-test-tools/-/issues/81
Comment 15 Martin Renvoize 2024-03-21 09:14:00 UTC
I think this is a better first step than my initial attempt.. I was considering a centralised file to list files we should be keeping tidy, but that felt more messy and likely to fall out of maintenance too...

Last step I reckon is to add a QA check to ensure new JS files contain the /* keep tidy */ flag. :)
Comment 16 Martin Renvoize 2024-03-21 09:34:50 UTC
Created attachment 163591 [details] [review]
Bug 36374: Tidy place_booking_modal.js according to our prettier config

This was somehow missed when the file was first added.. my mistake.
Comment 17 Martin Renvoize 2024-03-21 09:34:53 UTC
Created attachment 163592 [details] [review]
Bug 36374: Move modal js under /modals

I also didn't stick to the more recent switch to putting modal js under
a /modals dirctory.
Comment 18 Martin Renvoize 2024-03-21 09:34:55 UTC
Created attachment 163593 [details] [review]
Bug 36374: Ensure js/modals/*.js files are tidy
Comment 19 Martin Renvoize 2024-03-21 09:34:58 UTC
Created attachment 163594 [details] [review]
Bug 36374: flag JS files to keep tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2024-03-21 09:35:00 UTC
Created attachment 163595 [details] [review]
Bug 36374: Keep tidy files from 'fetch' directory

And tidy them

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2024-03-21 09:35:03 UTC
Created attachment 163596 [details] [review]
Bug 36374: Also tidy ticket and concern modals js

This patch does the promised follow-up to tidy the two other recently
added js files for modals. As I'm the main maintainer of those to date
I'm happy to rebase any patches already submitted atop this bug.
Comment 22 Martin Renvoize 2024-03-21 09:35:52 UTC
Given Jonathans input here I've signed off his follow-ups and think we're OK to go for at least Signed off.
Comment 23 Marcel de Rooy 2024-04-12 06:57:45 UTC
Sounds like enh to me?
Comment 24 Pedro Amorim 2024-04-16 08:48:19 UTC
Hey, adding my opinion here.
This should be 2 different bugs, one specifically as a bookings modal follow-up tidy and restructuring, and the other bug for the keep JS files tidy stuff. The former should be good to go but the latter may be worth discussing further.

I like the idea behind the *keep tidy* but I worry that having to specify each file manually going forward will not work in the long term, i.e. another thing to maintain/consider, will get forgotten, etc.
Personally I'd prefer we do it on a by directory basis, as Martin's originally suggested but perhaps at a lower i.e. koha-tmpl/intranet-tmpl/prog/js/modals/ instead of koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.
This could cause conflicts for other work sitting in bugzilla for starters but could also be seen as a middle ground between specifying each file individually and tidying the whole code base at once. Either that or make sure newly created .js files have the *keep tidy* at the top.
Comment 25 Jonathan Druart 2024-04-16 11:14:25 UTC
I would not block it now that we have the QA script and the git hook that have been published already.
We need to start with something, we can improve later if needed. The more we wait the more we will lack this tidy check.
Comment 26 Jonathan Druart 2024-04-16 11:15:40 UTC
Additionally having it on directory basis is less flexible. We can still have both, and so refine later if needed ;)
Comment 27 Martin Renvoize 2024-04-17 14:06:51 UTC
I'm going to go ahead and set this one to PQA.. we've had some good eyes and fair opinions.  I think it's the first step to cleaner JS overall and I agree I'd like to work hard to get further in the medium term.. but we have to be pragmatic and start somewhere.
Comment 28 Katrin Fischer 2024-04-29 07:02:19 UTC
Please rebase!

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 36374: Tidy place_booking_modal.js according to our prettier config
Applying: Bug 36374: Move modal js under /modals
Applying: Bug 36374: Ensure js/modals/*.js files are tidy
Applying: Bug 36374: flag JS files to keep tidy
Applying: Bug 36374: Keep tidy files from 'fetch' directory
Applying: Bug 36374: Also tidy ticket and concern modals js
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/modals/add_catalog_concern.js).

error: could not build fake ancestor

Patch failed at 0001 Bug 36374: Also tidy ticket and concern modals js
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-36374-Also-tidy-ticket-and-concern-modals-js-e_l9qfib.patch
Comment 29 Martin Renvoize 2024-04-29 11:48:12 UTC
Created attachment 165732 [details] [review]
Bug 36374: Tidy place_booking_modal.js according to our prettier config

This was somehow missed when the file was first added.. my mistake.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 30 Martin Renvoize 2024-04-29 11:48:14 UTC
Created attachment 165733 [details] [review]
Bug 36374: Move modal js under /modals

I also didn't stick to the more recent switch to putting modal js under
a /modals dirctory.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 31 Martin Renvoize 2024-04-29 11:48:17 UTC
Created attachment 165734 [details] [review]
Bug 36374: Ensure js/modals/*.js files are tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 32 Martin Renvoize 2024-04-29 11:48:19 UTC
Created attachment 165735 [details] [review]
Bug 36374: flag JS files to keep tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 33 Martin Renvoize 2024-04-29 11:48:22 UTC
Created attachment 165736 [details] [review]
Bug 36374: Keep tidy files from 'fetch' directory

And tidy them

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 34 Martin Renvoize 2024-04-29 11:48:25 UTC
Created attachment 165737 [details] [review]
Bug 36374: Also tidy ticket and concern modals js

This patch does the promised follow-up to tidy the two other recently
added js files for modals. As I'm the main maintainer of those to date
I'm happy to rebase any patches already submitted atop this bug.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 35 Martin Renvoize 2024-04-29 11:55:32 UTC
Created attachment 165738 [details] [review]
Bug 36374: Tidy place_booking_modal.js according to our prettier config

This was somehow missed when the file was first added.. my mistake.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 36 Martin Renvoize 2024-04-29 11:55:35 UTC
Created attachment 165739 [details] [review]
Bug 36374: Move modal js under /modals

I also didn't stick to the more recent switch to putting modal js under
a /modals dirctory.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 37 Martin Renvoize 2024-04-29 11:55:37 UTC
Created attachment 165740 [details] [review]
Bug 36374: Ensure js/modals/*.js files are tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 38 Martin Renvoize 2024-04-29 11:55:40 UTC
Created attachment 165741 [details] [review]
Bug 36374: flag JS files to keep tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 39 Martin Renvoize 2024-04-29 11:55:44 UTC
Created attachment 165742 [details] [review]
Bug 36374: Keep tidy files from 'fetch' directory

And tidy them

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 40 Martin Renvoize 2024-04-29 11:55:47 UTC
Created attachment 165743 [details] [review]
Bug 36374: Also tidy ticket and concern modals js

This patch does the promised follow-up to tidy the two other recently
added js files for modals. As I'm the main maintainer of those to date
I'm happy to rebase any patches already submitted atop this bug.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 41 Katrin Fischer 2024-04-29 13:20:38 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 42 Fridolin Somers 2024-05-24 10:00:22 UTC
Not backported to 23.11.x
Comment 43 Aude Charillon 2024-07-03 09:05:31 UTC
No changes to the Manual needed.