moqui / mantle-usl

Mantle Universal Service Library
http://www.moqui.org/mantle.html
Other
28 stars 60 forks source link

Long message in OrderInfoServices.check#OrderPreApprove cannot be localized #98

Closed jenshp closed 6 years ago

jenshp commented 6 years ago

Messages exceeding 255 characters cannot be localized as of restrictions in database and L10nFacadeImpl.localize() implementation. Service OrderInfoServices.check#OrderPreApprove has one message that exceed 255 characters, currently on line 612 with 276 characters: <iterate list="nearItemList" entry="nearItem"><script>approveWarnings.add(ec.resource.expand( 'Found similar order ${nearOrder.orderId}:${nearOrder.orderPartSeqId} placed ${ec.l10n.format(nearOrder.placedDate, null)} (${ec.l10n.format(diffHours, "0.0")}h ${isFuture ? "after" : "before"}) with ${ec.l10n.format(nearItem.quantity, "0.##")} of product ${nearItem.productId}', null))</script></iterate>

jonesde commented 6 years ago

The current behavior is that ec.resource.expand() only tries to call ec.l10n.localize() if the input string is shorter than 256 characters (literal logic in ResourceFacadeImpl.groovy, currently on line 501). If that were changed the next issue would be in L10nFacadeImpl.java where it throws an exception on line 57 if the original string is longer than 255 characters. If that were changed the next issue would be that the LocalizedMessage.original field is a text-medium (this is the one you've proposed changing a couple times, though without changes logic in the other 2 places it would have no effect).

There are a couple of approaches I can think of for framework changes:

  1. change across various parts of the framework code along with the LocalizedMessage entity definition to support longer original strings; this is easy enough to do but has database performance and storage size implications, especially when you have large numbers of localized messages

  2. instead of skipping longer original strings in resource.expand() and throwing an exception in l10n.localize() it could trim the original string to 255 characters and look for that in the database; this is even easier to do and has a smaller ripple effect than option 1, but would make localization data sets very ugly (option 1 results in ugly data sets too with very long original strings)

Part of the reason this constraint was part of the original design was for database size and index performance, but a more important reason is that while using the default language actual text as the original is a nice pattern and saves work when internationalizing applications it can lead to bad patterns, like using very long strings as localization keys.

This case is a great example of using the default language strings as keys. It is long, ugly, and the worst part: it is likely to change over time which would invalidate/break all prior translation records.

I'm tempted to go the other way in the framework and either log a warning or throw an exception in ec.resource.expand() if the original string is longer than 255 characters. This pushes developers to use the better approach as messages get too long: use artificial keys!

Artificial keys have the benefit of being short, simple, and stable. For fixing this particular issue for the various messages in OrderInfoServices my opinion is the best fix is to change it to use all artificial keys. This is something that should really be done for all of the longer messages through mantle-usl (I haven't researched this, but my guess is there are dozens or hundreds that should be changed to use artificial message keys). This involves adding LocalizedMessage records in the 'default' locale to seed data (similar to that various record name templates, like PartyNameTemplate, ProductNameTemplate, etc), and changing the code to use them.

Your thoughts?

jenshp commented 6 years ago

I agree that the best solution for these cases is to use artificial keys. If we define some convention to name them, I could help, starting with OrderInfoServices.check#OrderPreApprove.

jonesde commented 6 years ago

I started making changes in commit 02f3c32, still many more to do.

One thing to note is that this data is in an 'install' file which is normally only loaded on initial setup. Existing systems will need to have this loaded, and I added a note about this in the ReleaseNotes.txt file.

jonesde commented 6 years ago

Closing as part of general cleanup for the move to HiveMind and for no recent activity on this issue. If this comes up again or anyone wants to pursue it feel free to create a request on moqui.org. For more information see:

https://www.moqui.org/m/docs/moqui/Community+Guide