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.
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.
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.
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.
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
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.
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.
Created attachment 163560 [details] [review] Bug 36374: Ensure js/modals/*.js files are tidy
/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.
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)
Created attachment 163587 [details] [review] Bug 36374: flag JS files to keep tidy
Created attachment 163588 [details] [review] Bug 36374: flag JS files to keep tidy
Created attachment 163589 [details] [review] Bug 36374: Keep tidy files from 'fetch' directory And tidy them
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.
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
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. :)
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.
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.
Created attachment 163593 [details] [review] Bug 36374: Ensure js/modals/*.js files are tidy
Created attachment 163594 [details] [review] Bug 36374: flag JS files to keep tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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.
Given Jonathans input here I've signed off his follow-ups and think we're OK to go for at least Signed off.
Sounds like enh to me?
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.
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.
Additionally having it on directory basis is less flexible. We can still have both, and so refine later if needed ;)
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.
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
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Pushed for 24.05! Well done everyone, thank you!
Not backported to 23.11.x
No changes to the Manual needed.