Bug 25409 - Required dropdown missing "required" class near label
Summary: Required dropdown missing "required" class near label
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Slava Shishkin
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2020-05-07 10:22 UTC by Peter Vashchuk
Modified: 2020-11-30 21:47 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Screenshot of missing 'required' class (161.50 KB, image/png)
2020-05-07 10:22 UTC, Peter Vashchuk
Details
Bug 25409: Add missing "required" class to template (1.60 KB, patch)
2020-05-07 11:32 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25409: Add missing "required" class to template (1.65 KB, patch)
2020-05-07 11:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25409: Add missing "required" class to template (1.71 KB, patch)
2020-05-08 06:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25409: Add missing “Required” text and “required” classes (2.43 KB, patch)
2020-05-10 15:22 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25409: Add missing "Required" text and "required" classes (2.42 KB, patch)
2020-05-11 11:13 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2020-05-07 10:22:33 UTC
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
Comment 1 Slava Shishkin 2020-05-07 11:32:04 UTC
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.
Comment 2 Owen Leonard 2020-05-07 11:57:14 UTC
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>
Comment 3 Katrin Fischer 2020-05-08 06:27:09 UTC
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>
Comment 4 Katrin Fischer 2020-05-08 06:27:36 UTC
Welcome, Slava!
Comment 5 Slava Shishkin 2020-05-08 17:41:59 UTC
(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)
Comment 6 Katrin Fischer 2020-05-08 18:00:17 UTC
If you want to, we could add mentored-by: lines to the patch to acknowledge their help.
Comment 7 Andrii Nugged 2020-05-10 08:19:26 UTC
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.
Comment 8 Andrii Nugged 2020-05-10 08:22:50 UTC
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.
Comment 9 Slava Shishkin 2020-05-10 15:22:38 UTC
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.
Comment 10 Martin Renvoize 2020-05-11 10:54:55 UTC
(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.
Comment 11 Katrin Fischer 2020-05-11 11:13:20 UTC
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>
Comment 12 Katrin Fischer 2020-05-11 11:15:36 UTC
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 :)
Comment 13 Katrin Fischer 2020-05-11 11:16:09 UTC
Ah, please beware: “required” wasn't liked by git-bz, I changed to "required" :)
Comment 14 Andrii Nugged 2020-05-12 06:41:23 UTC
(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 :).
Comment 15 Andrii Nugged 2020-05-12 06:42:15 UTC
(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 :).
Comment 16 Andrii Nugged 2020-05-12 06:45:44 UTC
(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 :) )
Comment 17 Martin Renvoize 2020-05-12 10:24:17 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Katrin Fischer 2020-05-12 21:45:07 UTC
 
> noted as well. For three of us :).

:)
Comment 19 Joy Nelson 2020-05-13 19:21:00 UTC
pushed to 19.11.x for 19.11.06
Comment 20 Lucas Gass 2020-05-15 22:46:06 UTC
backported to 19.05.x for 19.05.11