Bug 35193 - Rename "Message subject" to "File name" when module "Reports" was chosen
Summary: Rename "Message subject" to "File name" when module "Reports" was chosen
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 34456
Blocks:
  Show dependency treegraph
 
Reported: 2023-10-30 20:27 UTC by Katrin Fischer
Modified: 2023-12-12 08:44 UTC (History)
3 users (show)

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


Attachments
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen (2.13 KB, patch)
2023-10-31 10:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen (2.31 KB, patch)
2023-11-04 12:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35193: Reload page on notice edit when switching Koha module (1.09 KB, patch)
2023-11-10 09:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen (2.31 KB, patch)
2023-11-10 18:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35193: Reload page on notice edit when switching Koha module (1.15 KB, patch)
2023-11-10 18:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 35193: Hide all non-print transports for report templates (1.53 KB, patch)
2023-11-13 13:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen (2.37 KB, patch)
2023-11-13 15:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35193: Reload page on notice edit when switching Koha module (1.21 KB, patch)
2023-11-13 15:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35193: Hide all non-print transports for report templates (1.59 KB, patch)
2023-11-13 15:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35193: (QA follow-up): Add missing opening div panel (1.51 KB, patch)
2023-11-13 15:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35193: Show 'File name' for Print only, keep 'Email' and 'SMS' transport types as options for 'Report' notices (1.84 KB, patch)
2023-11-13 15:34 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-10-30 20:27:30 UTC
I think this would make it a lot more obvious that the message name can/should be used to determine the file name and format.
Comment 1 Kyle M Hall 2023-10-31 10:37:29 UTC
Created attachment 158111 [details] [review]
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Choose to make or edit a reports notice/slip
4) Note the "Message subject" label has been replaced with "File name"
   for new and existing reports notices
Comment 2 Katrin Fischer 2023-11-04 12:25:29 UTC
Created attachment 158375 [details] [review]
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen

This changes the form for adding new notices so the
"Message subject" is labelled as "File name" when
the Reports module was chosen.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Choose to make or edit a reports notice/slip
4) Note the "Message subject" label has been replaced with "File name"
   for new and existing reports notices

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Katrin Fischer 2023-11-04 12:26:21 UTC
I think this is a good improvement, but I see a problem I hadn't thought of before: when editing or creating a new notice/slip, you can change the module on top and then the labels are not changing with it.

Should we add the JS to achieve this?
Comment 4 Kyle M Hall 2023-11-08 11:16:31 UTC
(In reply to Katrin Fischer from comment #3)
> I think this is a good improvement, but I see a problem I hadn't thought of
> before: when editing or creating a new notice/slip, you can change the
> module on top and then the labels are not changing with it.
> 
> Should we add the JS to achieve this?

I just tested on master and when I change the module at the top the page reloads and updates the language just as you describe!
Comment 5 Tomás Cohen Arazi 2023-11-08 20:45:55 UTC
(In reply to Kyle M Hall from comment #4)
> (In reply to Katrin Fischer from comment #3)
> > I think this is a good improvement, but I see a problem I hadn't thought of
> > before: when editing or creating a new notice/slip, you can change the
> > module on top and then the labels are not changing with it.
> > 
> > Should we add the JS to achieve this?
> 
> I just tested on master and when I change the module at the top the page
> reloads and updates the language just as you describe!

@katrin can you re-check?
Comment 6 Katrin Fischer 2023-11-08 21:02:49 UTC
I'll try to get to this again asap, but not unhappy if someone else wants/can take over!
Comment 7 Pedro Amorim 2023-11-09 17:50:22 UTC
Bug 34136 states that the template needs to be added to the "Print" area, but testing this, this patch is also changing for the "E-mail" and "SMS" areas, it seems unintended.

(In reply to Kyle M Hall from comment #4)
> (In reply to Katrin Fischer from comment #3)
> > I think this is a good improvement, but I see a problem I hadn't thought of
> > before: when editing or creating a new notice/slip, you can change the
> > module on top and then the labels are not changing with it.
> > 
> > Should we add the JS to achieve this?
> 
> I just tested on master and when I change the module at the top the page
> reloads and updates the language just as you describe!

Labels are changing for me correctly when changing the module on top. I can't reproduce what Katrin describes here.
Comment 8 Katrin Fischer 2023-11-09 20:25:01 UTC
This is very odd, but the page doesn't reload for me :(

Maybe I found the difference:

* Edit an existing notice
* Change the module on top
* Verify page isn't reloaded, labels don't change
Comment 9 Pedro Amorim 2023-11-10 09:23:59 UTC
(In reply to Katrin Fischer from comment #8)
> This is very odd, but the page doesn't reload for me :(
> 
> Maybe I found the difference:
> 
> * Edit an existing notice
> * Change the module on top
> * Verify page isn't reloaded, labels don't change

Interesting, yes, I can reproduce that.
Comment 10 Pedro Amorim 2023-11-10 09:31:24 UTC
Created attachment 158795 [details] [review]
Bug 35193: Reload page on notice edit when switching Koha module

Test plan, k-t-d:
1) Edit an existing notice, e.g.:
/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=&module=report&code=ACCOUNTS_SUMMARY
2) Change the Koha module dropdown value
3) Notice the page doesnt refresh
4) Apply patch. Repeat. Notice it refreshes now.
Comment 11 Katrin Fischer 2023-11-10 18:31:06 UTC
Back here...
Comment 12 Tomás Cohen Arazi 2023-11-10 18:40:07 UTC
String freeze already. We can fix this after the release.
Comment 13 Katrin Fischer 2023-11-10 18:40:49 UTC
Created attachment 158814 [details] [review]
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen

This changes the form for adding new notices so the
"Message subject" is labelled as "File name" when
the Reports module was chosen.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Choose to make or edit a reports notice/slip
4) Note the "Message subject" label has been replaced with "File name"
   for new and existing reports notices

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2023-11-10 18:40:52 UTC
Created attachment 158815 [details] [review]
Bug 35193: Reload page on notice edit when switching Koha module

Test plan, k-t-d:
1) Edit an existing notice, e.g.:
/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=&module=report&code=ACCOUNTS_SUMMARY
2) Change the Koha module dropdown value
3) Notice the page doesnt refresh
4) Apply patch. Repeat. Notice it refreshes now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2023-11-10 18:41:13 UTC
It looks like we only have my sign-off so far, so not moving to PQA.
Comment 16 Pedro Amorim 2023-11-11 13:41:01 UTC
(In reply to Pedro Amorim from comment #7)
> Bug 34136 states that the template needs to be added to the "Print" area,
> but testing this, this patch is also changing for the "E-mail" and "SMS"
> areas, it seems unintended.

I'm ready to SO but can we confirm if the above is intended? It seems off that a "E-mail" or "SMS" section would be labeled as "File name" instead of "Message subject"

I can sort of see it making sense for "Print" as we're downloading the "Print" render of the notice template, and I haven't thought about this long enough, but wouldn't a new distinct "File" section be more appropriate for this?
Comment 17 Kyle M Hall 2023-11-13 13:55:21 UTC
Created attachment 158877 [details] [review]
Bug 35193: Hide all non-print transports for report templates
Comment 18 Kyle M Hall 2023-11-13 13:56:50 UTC
(In reply to Pedro Amorim from comment #16)
> (In reply to Pedro Amorim from comment #7)
> > Bug 34136 states that the template needs to be added to the "Print" area,
> > but testing this, this patch is also changing for the "E-mail" and "SMS"
> > areas, it seems unintended.
> 
> I'm ready to SO but can we confirm if the above is intended? It seems off
> that a "E-mail" or "SMS" section would be labeled as "File name" instead of
> "Message subject"
> 
> I can sort of see it making sense for "Print" as we're downloading the
> "Print" render of the notice template, and I haven't thought about this long
> enough, but wouldn't a new distinct "File" section be more appropriate for
> this?

The intention of the feature is to only use the "print" transport. I've added a followup so that only the print transport shows up for report notice templates. I think that solves the issue nicely! If you sign it, we can use Katrin's sign-off as the QA signoff at her discretion.
Comment 19 Pedro Amorim 2023-11-13 15:27:27 UTC
Created attachment 158881 [details] [review]
Bug 35193: Rename "Message subject" to "File name" when module "Reports" was chosen

This changes the form for adding new notices so the
"Message subject" is labelled as "File name" when
the Reports module was chosen.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Choose to make or edit a reports notice/slip
4) Note the "Message subject" label has been replaced with "File name"
   for new and existing reports notices

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 20 Pedro Amorim 2023-11-13 15:27:30 UTC
Created attachment 158882 [details] [review]
Bug 35193: Reload page on notice edit when switching Koha module

Test plan, k-t-d:
1) Edit an existing notice, e.g.:
/cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=&module=report&code=ACCOUNTS_SUMMARY
2) Change the Koha module dropdown value
3) Notice the page doesnt refresh
4) Apply patch. Repeat. Notice it refreshes now.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 21 Pedro Amorim 2023-11-13 15:27:32 UTC
Created attachment 158883 [details] [review]
Bug 35193: Hide all non-print transports for report templates

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 22 Pedro Amorim 2023-11-13 15:27:35 UTC
Created attachment 158884 [details] [review]
Bug 35193: (QA follow-up): Add missing opening div panel

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 23 Pedro Amorim 2023-11-13 15:34:27 UTC
Created attachment 158885 [details] [review]
Bug 35193: Show 'File name' for Print only, keep 'Email' and 'SMS' transport types as options for 'Report' notices
Comment 24 Pedro Amorim 2023-11-13 15:36:35 UTC
I've added my sign-off, if @Katrin wants to turn hers to PQA.

(In reply to Kyle M Hall from comment #18)
> The intention of the feature is to only use the "print" transport. I've
> added a followup so that only the print transport shows up for report notice
> templates. I think that solves the issue nicely! If you sign it, we can use
> Katrin's sign-off as the QA signoff at her discretion.

I think it solves the issue nicely! However, previously we could have an "E-mail" type of notice for a Report and now we don't! I have added a new follow-up that maintains the "Email" and "SMS" options and only shows "File name" for "Print", keeping "Message subject" for the other two. Feel free to obsolete if this is not needed.
Comment 25 Kyle M Hall 2023-11-13 16:06:33 UTC
(In reply to Pedro Amorim from comment #24)
> I think it solves the issue nicely! However, previously we could have an
> "E-mail" type of notice for a Report and now we don't! I have added a new
> follow-up that maintains the "Email" and "SMS" options and only shows "File
> name" for "Print", keeping "Message subject" for the other two. Feel free to
> obsolete if this is not needed.

I'm not opposed to that, but I'm curious how you would use the email and sms options.
Comment 26 Pedro Amorim 2023-11-13 16:19:47 UTC
(In reply to Kyle M Hall from comment #25)
> (In reply to Pedro Amorim from comment #24)
> > I think it solves the issue nicely! However, previously we could have an
> > "E-mail" type of notice for a Report and now we don't! I have added a new
> > follow-up that maintains the "Email" and "SMS" options and only shows "File
> > name" for "Print", keeping "Message subject" for the other two. Feel free to
> > obsolete if this is not needed.
> 
> I'm not opposed to that, but I'm curious how you would use the email and sms
> options.

This is why I said feel free to obsolete =D I don't know the feature in-depth but I would imagine you would also be able to send a report notice to an e-mail (as with other notices types), additionally to have the option to download it, if not please ignore it.
Comment 27 Kyle M Hall 2023-11-13 16:34:35 UTC
(In reply to Pedro Amorim from comment #26)
> (In reply to Kyle M Hall from comment #25)
> > (In reply to Pedro Amorim from comment #24)
> > > I think it solves the issue nicely! However, previously we could have an
> > > "E-mail" type of notice for a Report and now we don't! I have added a new
> > > follow-up that maintains the "Email" and "SMS" options and only shows "File
> > > name" for "Print", keeping "Message subject" for the other two. Feel free to
> > > obsolete if this is not needed.
> > 
> > I'm not opposed to that, but I'm curious how you would use the email and sms
> > options.
> 
> This is why I said feel free to obsolete =D I don't know the feature
> in-depth but I would imagine you would also be able to send a report notice
> to an e-mail (as with other notices types), additionally to have the option
> to download it, if not please ignore it.

Thanks!
I'm going to deprecate it for the time being because that is not a function the feature supports at this time, though it would make for a great followup enhancement!
Comment 28 Katrin Fischer 2023-11-13 20:24:07 UTC
I agree, I think showing only the option that works is better for now. I'll try to test, but feel free to beat me to it :)
Comment 29 Pedro Amorim 2023-11-14 09:07:34 UTC
(In reply to Katrin Fischer from comment #28)
> I agree, I think showing only the option that works is better for now. I'll
> try to test, but feel free to beat me to it :)

Hey, I have tested and signed-off =D
Comment 30 Katrin Fischer 2023-11-14 09:15:27 UTC
(In reply to Pedro Amorim from comment #29)
> (In reply to Katrin Fischer from comment #28)
> > I agree, I think showing only the option that works is better for now. I'll
> > try to test, but feel free to beat me to it :)
> 
> Hey, I have tested and signed-off =D

I meant don't wait up for me with QA, but I'll try to come back to this next chance I get.
Comment 31 Katrin Fischer 2023-12-12 08:44:05 UTC
(In reply to Katrin Fischer from comment #30)
> (In reply to Pedro Amorim from comment #29)
> > (In reply to Katrin Fischer from comment #28)
> > > I agree, I think showing only the option that works is better for now. I'll
> > > try to test, but feel free to beat me to it :)
> > 
> > Hey, I have tested and signed-off =D
> 
> I meant don't wait up for me with QA, but I'll try to come back to this next
> chance I get.

I believe I didn't wait for the page reload, should be an easy QA now :)