Bug 10409

Summary: Do not show course entry form if no departments are found
Product: Koha Reporter: Owen Leonard <oleonard>
Component: Course reservesAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: gmcharlt, katrin.fischer, kyle
Version: Main   
Hardware: All   
OS: All   
URL: /cgi-bin/koha/course_reserves/course.pl
Change sponsored?: --- Patch complexity: String patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 10409 - Do not show course entry form if no departments are found
Bug 10409 - Do not show course entry form if no departments are found
Bug 10409 - Do not show course entry form if no departments are found
[PASSED QA] Bug 10409 - Do not show course entry form if no departments are found
Bug 10409: Follow up - improving strings for easier translation
Bug 10409: Follow up - improving strings for easier translation

Description Owen Leonard 2013-06-04 19:48:28 UTC
The course reserves entry form shows an error message instead of a form field for the department if no departments are defined: "No DEPARTMENT authorised values found! Please create one or more authorised values with the category DEPARTMENT."

If you can't fill out the form because there are no departments, there's no reason to display the form. Otherwise people might fill in the rest of the fields and find they can't submit the form.

It would be better to show the error message alone on the page and link the user to the authorized values page.
Comment 1 Owen Leonard 2013-06-05 12:45:01 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2013-06-07 12:33:46 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2013-06-28 18:42:10 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2013-06-30 14:50:57 UTC
Created attachment 19298 [details] [review]
[PASSED QA] Bug 10409 - Do not show course entry form if no departments are found

The course reserves entry form should not be shown if there are no
DEPARTMENT authorized values, since this prevents the form from being
submitted. This patch replaces the form with an error message when no
DEPARTMENT authorized values are found.

Also corrected:

- Corrected grid structure for more standard display
- Converted labels with no corresponding inputs to <span class="label">
- Closed unclosed tags
- Corrected incorrect capitalization

This patch contains whitespace changes, so please ignore whitespace when
examining changes.

To test, delete any DEPARTMENT authorized values, if present. Create a
new course in Course Reserves. You should see a warning that no
DEPARTMENT values were found.

If you are logged in with the correct permission, the warning should
contain a link to the correct authorized value page. If you do no, the
warning should refer the problem to an administrator.

After creating one or more DEPARTMENT values, the form should display
and submit correctly.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
After this patch has been applied, I get an error message and no form
if I don't have any DEPARTMENTs defined, which makes sense given that
the form can't be submitted without a DEPARTMENT.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
You now see a useful error message, when no departments have been
defined. Else you are taken to the correct form.
Comment 5 Katrin Fischer 2013-06-30 14:52:20 UTC
There are still some things that are a bit problematic: 
<legend>[% IF course_id %] Edit [% ELSE %] Create [% END %] course</legend>
'Lazy' constructs like that make it unnecessarily hard to translators, 
because grammar works different in different languages and having single 
strings like that in a predefined order is not nice. Even in German it does
not really work.
Comment 6 Katrin Fischer 2013-06-30 14:59:47 UTC Comment hidden (obsolete)
Comment 7 Galen Charlton 2013-07-01 02:09:55 UTC
(In reply to Owen Leonard from comment #0)
> The course reserves entry form shows an error message instead of a form
> field for the department if no departments are defined: "No DEPARTMENT
> authorised values found! Please create one or more authorised values with
> the category DEPARTMENT."

A question about the premise of this patch, possibly for Kyle: is there a functional reason why setting a department is absolutely required?  If so, why is courses.department a nullable column?
Comment 8 Galen Charlton 2013-07-02 13:34:29 UTC
Setting to "In discussion" pending a response to the question posed in comment 7.
Comment 9 Kyle M Hall 2013-07-02 13:54:05 UTC
(In reply to Galen Charlton from comment #7)
> (In reply to Owen Leonard from comment #0)
> > The course reserves entry form shows an error message instead of a form
> > field for the department if no departments are defined: "No DEPARTMENT
> > authorised values found! Please create one or more authorised values with
> > the category DEPARTMENT."
> 
> A question about the premise of this patch, possibly for Kyle: is there a
> functional reason why setting a department is absolutely required?  If so,
> why is courses.department a nullable column?

There is no functional reason to require the setting of a department. It was merely a requirement of the specification I was following ( where the functionality was based on another implementation of course reserves which made Department a required field ). Making department an optional value would do no harm. Of course, the same could be said for the "Name" field, so it's more of a philosophical question than anything. I guess the question is "In practice, will anyone want to create courses *without* course departments?".

So, we can either leave department as a required field, or make it optionally require via a system preference. What are your feelings on those options?
Comment 10 Katrin Fischer 2013-07-02 13:58:58 UTC
When we alrady discuss required fields... I wondered why the course number is required. I think the way course reserves work here, normally they just give the name of the professor or course without a number.
Comment 11 Kyle M Hall 2013-07-02 14:03:45 UTC
(In reply to Katrin Fischer from comment #10)
> When we alrady discuss required fields... I wondered why the course number
> is required. I think the way course reserves work here, normally they just
> give the name of the professor or course without a number.

Katrin, basically the same answer as before. It was in the spec, it doesn't *really* have to be requiired, and we could make it being a requirement optional via a system preference.
Comment 12 Owen Leonard 2013-07-15 16:42:27 UTC
I think if changing the required fields is something that libraries want a new bug should be created. I don't think the discussion should hold up this patch.
Comment 13 Katrin Fischer 2013-07-15 19:59:01 UTC
Hm, still think it's a bit funny to have unrequired (by the system) fields required. I think the question here is if we push Owen's fix or remove the required attribute from the field?
Comment 14 Kyle M Hall 2013-07-15 20:09:10 UTC
(In reply to Katrin Fischer from comment #13)
> Hm, still think it's a bit funny to have unrequired (by the system) fields
> required. I think the question here is if we push Owen's fix or remove the
> required attribute from the field?

Owen's fix does not change the functionality of the system, whereas removing the required attribute would. I would suggest we push this along, and if you'd like to see the option to make any of the given field values required/optional, we should file a separate bug for that. It really has no bearing on this particular bug. Both pre and post patch behaviors prohibit the creation of a course without a department, and only the presentation has changed.
Comment 15 Kyle M Hall 2013-07-15 20:10:54 UTC
Created attachment 19683 [details] [review]
Bug 10409: Follow up - improving strings for easier translation

Fixing this line:
<legend>[% IF course_id %] Edit [% ELSE %] Create [% END %] course</legend>

As grammar works different in different languages, having single
strings like that in a predefined order makes having a nice translation
unnecessarily hard.

This will make it a little easier:
<legend>[% IF course_id %]Edit course[% ELSE %]Create course[% END %]</legend>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Galen Charlton 2013-07-17 14:26:57 UTC
(In reply to Katrin Fischer from comment #13)
> Hm, still think it's a bit funny to have unrequired (by the system) fields
> required. I think the question here is if we push Owen's fix or remove the
> required attribute from the field?

I've gone ahead and pushed Owen's patch (thanks, Owen!) and your follow-up (thanks, Katrin!), but from my POV, the question of what fields should be required remains open, so I've opened bug 10603.