Summary: | "Scalar" TT plugin no longer needed | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | Pushed to main --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dcook, m.de.rooy, martin.renvoize, tomascohen |
Version: | unspecified | Keywords: | release-notes-needed |
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: |
This patch set removes the Context TT plugin which only contained the Scalar method. This was only used a few times in the codebase. (We do not expect them to be used in notices on larger scale.)
The db revision will warn users for (adjusted) notices that contain this construct if any. Occurrences like [% Context.Scalar(x,y) %] can be replaced by [% x.y %]. If x.y returns a list, change to x.y.size.
|
|
Version(s) released in: |
25.11.00
|
Circulation function: | |
Bug Depends on: | 23166, 29844 | ||
Bug Blocks: | |||
Attachments: |
Bug 30915: Remove Context.Scalar calls from templates
Bug 30915: Remove the TT plugin itself Bug 30915: Remove Context.Scalar calls from templates Bug 30915: Remove the TT plugin itself Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar Bug 30915: Remove Context.Scalar calls from templates Bug 30915: Remove the TT plugin itself Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar Bug 30915: Remove Context.Scalar calls from templates Bug 30915: Remove the TT plugin itself Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar Bug 30915: Fix spelling |
Description
Jonathan Druart
2022-06-07 07:50:18 UTC
Created attachment 184468 [details] [review] Bug 30915: Remove Context.Scalar calls from templates Test plan: git grep on "Context.Scalar" Go to book with holdable items in staff catalogue detail. Check if Place hold is seen in the horizontal toolbar above the title. Place an article request with a patron. Check the article requests tab on patron detail. Same for the article requests tab on moremember (Patron details). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184469 [details] [review] Bug 30915: Remove the TT plugin itself Test plan: git grep Koha::Template::Plugin::Context git grep "\[% USE Context" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Quoting Martin from bug 28883: I don't hugely like the use of Context.Scalar in templates.. I agree with the change and things look ok. My only concern is: * Should we deprecate it first? (e.g. people might be using it in notices) * In the event we remove it (via deprecation window or not), shouldn't we provide an alternative approach at least in the documentation? I noticed the feature doesn't seem to have tests implemented. Created attachment 185108 [details] [review] Bug 30915: Remove Context.Scalar calls from templates Test plan: git grep on "Context.Scalar" Go to book with holdable items in staff catalogue detail. Check if Place hold is seen in the horizontal toolbar above the title. Place an article request with a patron. Check the article requests tab on patron detail. Same for the article requests tab on moremember (Patron details). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185109 [details] [review] Bug 30915: Remove the TT plugin itself Test plan: git grep Koha::Template::Plugin::Context git grep "\[% USE Context" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi (tcohen) from comment #4) > I agree with the change and things look ok. > > My only concern is: > > * Should we deprecate it first? (e.g. people might be using it in notices) > * In the event we remove it (via deprecation window or not), shouldn't we > provide an alternative approach at least in the documentation? > > I noticed the feature doesn't seem to have tests implemented. Thx for signing off here. Legitimate questions! I assume (..) that we will have not much people using this in notices based on the fact that we only used the construct in our codebase just a few times too. But I think that we definitely should warn people who do use it in notices. Since this will probably be a small number and manually replacing them should not be so hard, I tend to think that a warning dbrev should be sufficient. Will add that now. The refactoring pattern is Replace Context.Scalar( $x, $y ) by just $x.$y if you know that method y (like search and friends) will be a scalar (resultset). This was the case in the templates. If method y returns a list and was "Scalared" then you need to add a count to get at the same result. So in that case replace Context.Scalar( $x, $y ) by $x.$y.count. Note that this was probably not used like that, since using Scalar for that is unnecessary complexity for $x.$y.count already. (In reply to Marcel de Rooy from comment #7) > If method y returns a list and was "Scalared" then you need to add a count > to get at the same result. So in that case replace Context.Scalar( $x, $y ) > by $x.$y.count. Note that this was probably not used like that, since using > Scalar for that is unnecessary complexity for $x.$y.count already. Hmm. Within TT we should use size here. Created attachment 185117 [details] [review] Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar As requested on comment4. Test plan: Run the dbrev. Bonus: Add a Context.Scalar construct in a notice and rerun. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 186719 [details] [review] Bug 30915: Remove Context.Scalar calls from templates Test plan: git grep on "Context.Scalar" Go to book with holdable items in staff catalogue detail. Check if Place hold is seen in the horizontal toolbar above the title. Place an article request with a patron. Check the article requests tab on patron detail. Same for the article requests tab on moremember (Patron details). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 186720 [details] [review] Bug 30915: Remove the TT plugin itself Test plan: git grep Koha::Template::Plugin::Context git grep "\[% USE Context" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 186721 [details] [review] Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar As requested on comment4. Test plan: Run the dbrev. Bonus: Add a Context.Scalar construct in a notice and rerun. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 186724 [details] [review] Bug 30915: Remove Context.Scalar calls from templates Test plan: git grep on "Context.Scalar" Go to book with holdable items in staff catalogue detail. Check if Place hold is seen in the horizontal toolbar above the title. Place an article request with a patron. Check the article requests tab on patron detail. Same for the article requests tab on moremember (Patron details). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 186725 [details] [review] Bug 30915: Remove the TT plugin itself Test plan: git grep Koha::Template::Plugin::Context git grep "\[% USE Context" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 186726 [details] [review] Bug 30915: (QA follow-up) Add warning for notices with Context.Scalar As requested on comment4. Test plan: Run the dbrev. Bonus: Add a Context.Scalar construct in a notice and rerun. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 186727 [details] [review] Bug 30915: Fix spelling WARN spelling nWe ==> new Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> We overlapped, Tomas. I don't understand the QA failure, I am investigating it. The spelling check is not understanding \n is a escaped new line . (In reply to Jonathan Druart from comment #17) > We overlapped, Tomas. > > I don't understand the QA failure, I am investigating it. Fixed, https://gitlab.com/koha-community/qa-test-tools/-/commit/9c800d3c54c3ff11861d26f9f611c69c5bba6d9d (In reply to Tomás Cohen Arazi (tcohen) from comment #18) > The spelling check is not understanding \n is a escaped new line . I've fixed this one, see the last patch. Nice work everyone! Pushed to main for 25.11 |