Bug 5031 - CSV export profile form doesn't indicate that profile marcfields are required
Summary: CSV export profile form doesn't indicate that profile marcfields are required
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Bugs List
URL:
Keywords:
: 9960 (view as bug list)
Depends on: 10853
Blocks: 11333
  Show dependency treegraph
 
Reported: 2010-07-16 17:15 UTC by Owen Leonard
Modified: 2015-06-04 23:24 UTC (History)
8 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:


Attachments
Bug 5031: CSV profiles: content fields are required (6.54 KB, patch)
2013-09-10 15:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5031: CSV profiles: content fields are required (7.15 KB, patch)
2013-09-21 22:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 5031 - followup to correct tabs (2.68 KB, patch)
2013-09-21 22:09 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 5031: CSV profiles: content fields are required (7.29 KB, patch)
2013-12-03 11:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 5031 - followup to correct tabs (2.82 KB, patch)
2013-12-03 11:46 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 Owen Leonard 2010-07-16 17:15:01 UTC
If you try to save a CSV export profile without filling in profile marcfields, the page refreshes without saving any data and without showing any warnings. The form should at least indicate that it is a required field. A proper fix would display warnings if the profile was not saved because of missing data.
Comment 1 Owen Leonard 2012-05-17 19:35:02 UTC
In fact the script doesn't have any error handling at all. It will process inserts, updates, and deletes if all the required data is submitted:

if ($profile_name && $profile_content && $action) {
 ...


But if any of those doesn't exist nothing happens.
Comment 2 Jared Camins-Esakov 2013-03-31 12:59:47 UTC
*** Bug 9960 has been marked as a duplicate of this bug. ***
Comment 3 Jonathan Druart 2013-09-10 15:22:49 UTC Comment hidden (obsolete)
Comment 4 I'm just a bot 2013-09-18 10:58:38 UTC
Applying: Bug 5031: CSV profiles: content fields are required
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/tools/csv-profiles.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/csv-profiles.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/csv-profiles.tt
Patch failed at 0001 Bug 5031: CSV profiles: content fields are required
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 5 Jonathan Druart 2013-09-18 12:20:22 UTC
(In reply to I'm just a bot from comment #4)
Depend on bug 10853
Comment 6 Bernardo Gonzalez Kriegel 2013-09-21 22:08:34 UTC Comment hidden (obsolete)
Comment 7 Bernardo Gonzalez Kriegel 2013-09-21 22:09:51 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2013-09-22 20:07:47 UTC
Hi Jonathan,

moving on from bug 10853 to this one. The highlighting of the required fields is a good addition, but the formatting seems a bit non-standard. 
Maybe check Owen's work on bug 6209?

Putting 'In Discussion' and adding Owen in CC in hope for some feedback.
Comment 9 Jonathan Druart 2013-10-29 15:41:38 UTC
(In reply to Katrin Fischer from comment #8)
> Hi Jonathan,
> 
> moving on from bug 10853 to this one. The highlighting of the required
> fields is a good addition, but the formatting seems a bit non-standard. 
> Maybe check Owen's work on bug 6209?

Katrin, what do you consider as non-standard?
Comment 10 Katrin Fischer 2013-10-29 15:46:35 UTC
It looks really nice, but not like in other places of Koha :) But maybe Owen could take a look later?
Comment 11 Jonathan Druart 2013-10-30 10:00:59 UTC
(In reply to Katrin Fischer from comment #10)
> It looks really nice, but not like in other places of Koha :) But maybe Owen
> could take a look later?

I used the "required" attribute and the "required" class. The attribute is a new one (html5) and the required class is the way to do, I think.
I switch back to Signoff: since bug 10853 is pushed to master, I think this one should be in 3.14
I will quickly submit another patch if Owen considers something is wrong.
Comment 12 Katrin Fischer 2013-10-30 10:54:48 UTC
Yes, that's ok.
Comment 13 Katrin Fischer 2013-12-03 11:45:08 UTC
Created attachment 23257 [details] [review]
[PASSED QA] Bug 5031: CSV profiles: content fields are required

Some fields are required on editing/adding csv profiles.

Test plan:
- add a new csv profile (tools/csv-profiles.pl) without any value.
- save => error on empty fields.
- fill the "marc" content, select type="sql", fill the csv name input.
- save => error on sql content field.
- fill the sql content
- save => the csv profile is saved.
- retry for the edit form.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Comment: On top of both patches for Bug 10853

Works very well, now we get an alert with empty field
Saves correctly MARC and SQL profiles

Solved minor conflict introduced by tab followup on Bug 10853
Again small tab errors corrected in followup

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, looks a bit 'non standard' for Koha,
but improves usability of the form.
Comment 14 Katrin Fischer 2013-12-03 11:46:02 UTC
Created attachment 23258 [details] [review]
[PASSED QA] Bug 5031 - followup to correct tabs

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tiny QA follow-up corrcting whitespace to match coding
guidelines.
Comment 15 Galen Charlton 2013-12-03 17:03:14 UTC
Pushed to master.  Thanks, Jonathan!
Comment 16 Galen Charlton 2013-12-03 17:14:54 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Katrin Fischer from comment #10)
> > It looks really nice, but not like in other places of Koha :) But maybe Owen
> > could take a look later?
> 
> I used the "required" attribute and the "required" class. The attribute is a
> new one (html5) and the required class is the way to do, I think.
> I switch back to Signoff: since bug 10853 is pushed to master, I think this
> one should be in 3.14
> I will quickly submit another patch if Owen considers something is wrong.

I've opened bug 11333 and attached a patch that makes the two forms on this page use the jQuery validation plugin, which should address some of Katrin's concerns.

The way that this form handles editing existing profiles is decidedly not consistent with the rest of Koha, but that would be the topic of another bug.
Comment 17 Fridolin Somers 2013-12-13 15:32:10 UTC
Those patches has been pushed to 3.14.x, will be in 3.14.1.