Created attachment 104486 [details] Screenshot of missing 'required' class Select tag 'itemtype' in "onboardingstep5.tt" has 'required' attribute, accordingly to that, related label tag should also have 'required' class. Simple fix which slavashishkin@gmail.com will implement under my supervision. Similar cases I have found in history, which are pretty old: Bug 11812, Bug 4294, Bug 7670
Created attachment 104493 [details] [review] Bug 25409: Add missing "required" class to template Added "required" class to "itemtype" label in "onboardingstep5.tt" template. To test: 1) Start new instalation. During onboarding tool phase you will be requested to create a new circulation rule. 2) Observe that "itemtype" label has no "required" class. 3) Apply patch. 4) Repeat step 2 (you can reload circulation rule creation page). 5) Observe the error is gone.
Created attachment 104499 [details] [review] Bug 25409: Add missing "required" class to template Added "required" class to "itemtype" label in "onboardingstep5.tt" template. To test: 1) Start new instalation. During onboarding tool phase you will be requested to create a new circulation rule. 2) Observe that "itemtype" label has no "required" class. 3) Apply patch. 4) Repeat step 2 (you can reload circulation rule creation page). 5) Observe the error is gone. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 104556 [details] [review] Bug 25409: Add missing "required" class to template Added "required" class to "itemtype" label in "onboardingstep5.tt" template. To test: 1) Start new instalation. During onboarding tool phase you will be requested to create a new circulation rule. 2) Observe that "itemtype" label has no "required" class. 3) Apply patch. 4) Repeat step 2 (you can reload circulation rule creation page). 5) Observe the error is gone. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Welcome, Slava!
(In reply to Katrin Fischer from comment #4) > Welcome, Slava! Katrin, thanks for your welcoming in the community! (also thanks Peter for supervising, Nugged for involving, and both for training)
If you want to, we could add mentored-by: lines to the patch to acknowledge their help.
Katrin: It's more about teamwork then fame but one day all of us (me & my both trainees) we will be in contributors list – that will be achievement :), now we in a very intensive learning process how to become better Koha contributors mixed Finland-Ukraine team :). ... and this ticket is very simple... So our goal was more to go through the correct process of creation/testing/description/merging into master for simpler code, and it seems we doing well (Petro and Slava: thanks!) Let's proceed further.
Guys (Slava/Petro): please also repeat the same fix for "Units: days/hours" <select> dropdown field in the same template: it also has a "required" attribute, but the label also lost it's "required" class, and more: no "Required" post-tag <span>ed text, so I think you should add it too.
Created attachment 104634 [details] [review] Bug 25409: Add missing “Required” text and “required” classes Highlight text with red by adding “required” class to the label and added red “Required” text next to select field in "onboardingstep5.tt" template. To test: 1) Start new installation. During onboarding tool phase you will be requested to create a new circulation rule. 2) Observe that "Units" label has no "required" class and that there’s no “Required” text next to a select field. 3) Apply patch. 4) Repeat step 2 (you can reload circulation rule creation page). 5) Observe the error is gone.
(In reply to Katrin Fischer from comment #6) > If you want to, we could add mentored-by: lines to the patch to acknowledge > their help. I love to see these `Mentored-by: ` git trailers.. I think it really goes to show how much of a welcoming community we are and showcases us working together.. Great teamwork guys, nice to have you aboard.
Created attachment 104684 [details] [review] Bug 25409: Add missing "Required" text and "required" classes Highlight text with red by adding “required” class to the label and added red “Required” text next to select field in "onboardingstep5.tt" template. To test: 1) Start new installation. During onboarding tool phase you will be requested to create a new circulation rule. 2) Observe that "Units" label has no "required" class and that there’s no “Required” text next to a select field. 3) Apply patch. 4) Repeat step 2 (you can reload circulation rule creation page). 5) Observe the error is gone. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Hi Slava, usually once a patch has PQA it's better to open a new bug for a new issue, this way one can proceed while the other is processed. This was quite a small change, so I am using my QA manager powers to move this back to Passed QA immediately :)
Ah, please beware: “required” wasn't liked by git-bz, I changed to "required" :)
(In reply to Katrin Fischer from comment #13) > Ah, please beware: “required” wasn't liked by git-bz, I changed to > "required" :) yes, noted, sorry it was cross-messenger MacOS conversion, but now three of us know this so (high probability) won't happen again :).
(In reply to Katrin Fischer from comment #12) > Hi Slava, usually once a patch has PQA it's better to open a new bug for a > new issue, this way one can proceed while the other is processed. This was > quite a small change, so I am using my QA manager powers to move this back > to Passed QA immediately :) noted as well. For three of us :).
(In reply to Martin Renvoize from comment #10) > I love to see these `Mentored-by: ` git trailers.. I think it really goes to > show how much of a welcoming community we are and showcases us working > together.. okay, we will consider this. (oh, I should mention Joonas for mine earlier commits everywhere...) > Great teamwork guys, nice to have you aboard. ( /me glad to hear and melting in wide smile :) )
Nice work everyone! Pushed to master for 20.05
> noted as well. For three of us :). :)
pushed to 19.11.x for 19.11.06
backported to 19.05.x for 19.05.11