Bug 34833 - "order number" untranslatable when editing estimated delivery date
Summary: "order number" untranslatable when editing estimated delivery date
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Katrin Fischer
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-19 19:08 UTC by Caroline Cyr La Rose
Modified: 2023-10-17 17:24 UTC (History)
3 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:
23.11.00,23.05.05,22.11.11


Attachments
Bug 34833: Make "order number" in acq modal translatable (4.30 KB, patch)
2023-09-19 19:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 34833: Make "order number" in acq modal translatable (4.37 KB, patch)
2023-09-19 20:54 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 34833: Make "order number" in acq modal translatable (4.43 KB, patch)
2023-09-25 07:00 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
MAYBE FOLLOW UP Bug 34833: (follow-up): remove redundant spaces (2.85 KB, patch)
2023-09-25 07:00 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 34833: (follow-up): remove redundant spaces (2.83 KB, patch)
2023-09-25 20:27 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2023-09-19 19:08:37 UTC
I can't find the string to translate "order number" in the modal that appears when editing the estimated delivery date of an order.

The only string that looks like it is in late orders.

#. SCRIPT
#: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt:324
msgid "%s (order number %s)"
msgstr "%s (numéro de commande %s)"

To get there:
0. Make sure you have budgets and vendors set up

1. Create a basket (skip this if you already have an open basket)
   1.1. Go to Acquisitions
   1.2. Search for a vendor
   1.3. Click New > Basket
   1.4. Enter a name for the basket
   1.5. To make the tests easier, choose to create items when : cataloging the record
   1.6. Click Save

2. Add an order 
   2.1. Click Add to basket > From a new (empty) record
   2.2. Fill out the form, minimally
        - Enter a title
        - Enter a quantity
        - Choose a fund
        - Enter a vendor price
   2.3. Click Save

3. Next to the order line, click Edit under Estimated delivery date

--> The untranslatable string is in the title (green part) of the modal

"Edit delivery date" is translatable
"order number" is not translatable
Comment 1 Katrin Fischer 2023-09-19 19:59:14 UTC
Created attachment 155911 [details] [review]
Bug 34833: Make "order number" in acq modal translatable

The 'order number' was not translatable when editing the
estimated delivery date or notes from basket summary or
when editing the estimated delivery date from late orders.

To get there:
0. Make sure you have budgets and vendors set up

1. Create a basket (skip this if you already have an open basket)
   1.1. Go to acquisitions
   1.2. Search for a vendor
   1.3. Click New > basket
   1.4. Enter a name for the basket
   1.5. To make the tests easier, choose to create items when : cataloging the record
   1.6. Click save

2. Add an order
   2.1. Click add to basket > From a new (empty) record
   2.2. Fill out the form, minimally
        - Enter a title
        - Enter a quantity
        - Choose a fund
        - Enter a vendor price
   2.3. Click Save

3. Next to the order line, click Edit under Estimated delivery date
4. Verify that the 'order number <ordernumber>' shows on top of the modal
5. Edit internal/vendor note, verify it shows there as well
6. Close basket
7. Go to late orders
8. Edit estimated delivery date, text should show there as well
9. Apply patch
10. Run translation update script
11. Verify the string now appears in po files
12. Translate it
13. Install translation
14. Verify it shows nicely translated in all 3 mentioned spots
Comment 2 Caroline Cyr La Rose 2023-09-19 20:54:45 UTC
Created attachment 155914 [details] [review]
Bug 34833: Make "order number" in acq modal translatable

The 'order number' was not translatable when editing the
estimated delivery date or notes from basket summary or
when editing the estimated delivery date from late orders.

To get there:
0. Make sure you have budgets and vendors set up

1. Create a basket (skip this if you already have an open basket)
   1.1. Go to acquisitions
   1.2. Search for a vendor
   1.3. Click New > basket
   1.4. Enter a name for the basket
   1.5. To make the tests easier, choose to create items when : cataloging the record
   1.6. Click save

2. Add an order
   2.1. Click add to basket > From a new (empty) record
   2.2. Fill out the form, minimally
        - Enter a title
        - Enter a quantity
        - Choose a fund
        - Enter a vendor price
   2.3. Click Save

3. Next to the order line, click Edit under Estimated delivery date
4. Verify that the 'order number <ordernumber>' shows on top of the modal
5. Edit internal/vendor note, verify it shows there as well
6. Close basket
7. Go to late orders
8. Edit estimated delivery date, text should show there as well
9. Apply patch
10. Run translation update script
11. Verify the string now appears in po files
12. Translate it
13. Install translation
14. Verify it shows nicely translated in all 3 mentioned spots

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 3 Victor Grousset/tuxayo 2023-09-25 07:00:39 UTC
Created attachment 156143 [details] [review]
Bug 34833: Make "order number" in acq modal translatable

The 'order number' was not translatable when editing the
estimated delivery date or notes from basket summary or
when editing the estimated delivery date from late orders.

To get there:
0. Make sure you have budgets and vendors set up

1. Create a basket (skip this if you already have an open basket)
   1.1. Go to acquisitions
   1.2. Search for a vendor
   1.3. Click New > basket
   1.4. Enter a name for the basket
   1.5. To make the tests easier, choose to create items when : cataloging the record
   1.6. Click save

2. Add an order
   2.1. Click add to basket > From a new (empty) record
   2.2. Fill out the form, minimally
        - Enter a title
        - Enter a quantity
        - Choose a fund
        - Enter a vendor price
   2.3. Click Save

3. Next to the order line, click Edit under Estimated delivery date
4. Verify that the 'order number <ordernumber>' shows on top of the modal
5. Edit internal/vendor note, verify it shows there as well
6. Close basket
7. Go to late orders
8. Edit estimated delivery date, text should show there as well
9. Apply patch
10. Run translation update script
11. Verify the string now appears in po files
12. Translate it
13. Install translation
14. Verify it shows nicely translated in all 3 mentioned spots

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2023-09-25 07:00:41 UTC
Created attachment 156144 [details] [review]
MAYBE FOLLOW UP Bug 34833: (follow-up): remove redundant spaces
Comment 5 Victor Grousset/tuxayo 2023-09-25 07:11:14 UTC
Everything looks good I'm just suspicious about the space at the start there:
« (order number»

The concatenation to make $modalTitle already adds one space. So the HTML ends up with two.

It seems to come from this change: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=139839&action=diff

Which didn't have two steps to make the string so the space before «(order number» was necessary.

I tried without the additional space on the 3 places of this ticket and rendering looks good and HTML has only one space.
Proposal follow-up attached.
Comment 6 Katrin Fischer 2023-09-25 07:52:26 UTC
You are right about the follow-up. I did intend to "move" the space outside of translations, but forgot to remove it from the string.
Comment 7 Caroline Cyr La Rose 2023-09-25 14:51:10 UTC
I tried before and after the follow-up and did not see a difference in the interface.
Comment 8 Caroline Cyr La Rose 2023-09-25 14:53:51 UTC
to be precise, I meant that I only saw one space before the parenthesis (I checked by highlighting the text), with and without the follow-up patch... 

Maybe I'm misunderstanding what is being discussed...
Comment 9 Katrin Fischer 2023-09-25 14:55:28 UTC
(In reply to Caroline Cyr La Rose from comment #8)
> to be precise, I meant that I only saw one space before the parenthesis (I
> checked by highlighting the text), with and without the follow-up patch... 
> 
> Maybe I'm misunderstanding what is being discussed...

I think the browser collapses 2 into 1, but it's cleaner not to add them twice and the space at the beginning of a string might be problematic for translations too. 

I don't think this needs to be Failed QA?
Comment 10 Caroline Cyr La Rose 2023-09-25 15:07:52 UTC
(In reply to Katrin Fischer from comment #9)

> I don't think this needs to be Failed QA?

I think Victor maybe wanted some feedback on the follow-up since it's called "MAYBE FOLLOW UP ..."

Which is why I tried to test it with and without the follow up patch. I don't see any difference in the .po file either.

If it's only a clean code thing, it's fine since it doesn't change the behaviour.
If it's meant to change the string, it's (maybe?) not working.
Comment 11 Victor Grousset/tuxayo 2023-09-25 20:27:39 UTC
Created attachment 156193 [details] [review]
Bug 34833: (follow-up): remove redundant spaces
Comment 12 Victor Grousset/tuxayo 2023-09-25 20:42:20 UTC
(In reply to Katrin Fischer from comment #6)
> You are right about the follow-up. I did intend to "move" the space outside
> of translations, but forgot to remove it from the string.

Good, I just needed a confirmation about this then,
Works, makes sense, QA script happy, code looks good, passing QA :)

---

(In reply to Katrin Fischer from comment #9)
> I think the browser collapses 2 into 1

It indeed looks like that :)

> I don't think this needs to be Failed QA?

At first I was really not sure to have really found a mistake with the space thing. Hence the "maybe follow-up" and didn't know which status to use.

Then I saw it seemed to come from Bug 15348 and got almost sure it was a mistake so Failed QA seemed to fit enough. But still needed a confirmation.
Maybe "in discussion" fit better?

---

(In reply to Caroline Cyr La Rose from comment #10)
> I think Victor maybe wanted some feedback on the follow-up since it's called
> "MAYBE FOLLOW UP ..."

Yeah, either case it would have been able to move on after settling the space question.

> Which is why I tried to test it with and without the follow up patch. I
> don't see any difference in the .po file either.

Thanks for retesting.

> If it's only a clean code thing, it's fine since it doesn't change the
> behaviour.
> If it's meant to change the string, it's (maybe?) not working.

In either case, the translatable string was without a leading space. Nice that it can avoid duplication of strings :)
Comment 13 Tomás Cohen Arazi 2023-10-04 13:33:38 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-10-05 20:21:18 UTC
Pushed to 23.05.x for 23.05.05
Comment 15 Matt Blenkinsop 2023-10-17 17:24:38 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x