Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-9445

ContentWorker#findAlternateLocaleContent(Delegator, GenericValue, Locale) does not use fallback locale

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.04
    • Component/s: content
    • Labels:
      None
    • Flags:
      Patch

      Description

      Contrary to the UiLabels the ContentWorker#findAlternateLocaleContent(Delegator, GenericValue, Locale) function does not default to the fallback locale if no alternate content with the given locale is found, but returns the originial content. This proves to be an issue, if the root content is not in the fallback language.

      For instance: if the fallback locale is set to english, the ProductContent for the PRODUCT_NAME is in german, the alternate locale contents for the product name are english and spanish, then an italian visitor will see the page in english, but the product names in german.

      A fix for this issue is simple:
      If no alternate locale content for the requested locale is found, search for an alternate locale content with the locale configured in general.properties at locale.properties.fallback. If this one isn't found either we can still return the original content.

      1. OFBIZ-9445.patch
        2 kB
        Tobias Laufkötter
      2. OFBIZ-9445v2.patch
        1 kB
        Tobias Laufkötter
      3. OFBIZ-9445v3.patch
        3 kB
        Tobias Laufkötter
      4. OFBIZ-9445v3.patch
        3 kB
        Tobias Laufkötter

        Issue Links

          Activity

          Hide
          tlaufkoetter Tobias Laufkötter added a comment -

          Decided to mark it as a bug instead of an improvement, since this is not an improvement of an undesired quality, but a fix of unexpected behaviour.

          Show
          tlaufkoetter Tobias Laufkötter added a comment - Decided to mark it as a bug instead of an improvement, since this is not an improvement of an undesired quality, but a fix of unexpected behaviour.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Tobias,

          +1, I think you could use UtilProperties.getFallbackLocale()

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Tobias, +1, I think you could use UtilProperties.getFallbackLocale()
          Hide
          tlaufkoetter Tobias Laufkötter added a comment -

          So close...

          I updated the patch to use UtilProperties.getFallbackLocale().

          Show
          tlaufkoetter Tobias Laufkötter added a comment - So close... I updated the patch to use UtilProperties.getFallbackLocale().
          Hide
          tlaufkoetter Tobias Laufkötter added a comment -

          The function didn't recognise if the given content was already of the right locale. So I added it to the list of alternate locales to be taken into consideration.

          This case happens if the session locale string is "de_DE", but the content's locale string is "de".

          Show
          tlaufkoetter Tobias Laufkötter added a comment - The function didn't recognise if the given content was already of the right locale. So I added it to the list of alternate locales to be taken into consideration. This case happens if the session locale string is "de_DE", but the content's locale string is "de".
          Hide
          mbrohl Michael Brohl added a comment -

          Thanks Tobias,

          your fix is in

          trunk r1800780
          release16.11 r1800781

          Show
          mbrohl Michael Brohl added a comment - Thanks Tobias, your fix is in trunk r1800780 release16.11 r1800781
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          I reopen because we have weird tests issues popping. At 1st glance they don't seem related to the commit, but when we revert all tests pass. It could be test data locations or wrong tests, but for now it depends on this issue.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited I reopen because we have weird tests issues popping. At 1st glance they don't seem related to the commit, but when we revert all tests pass. It could be test data locations or wrong tests, but for now it depends on this issue.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          BTW Tobias, a small hint, you don't need to rename your patches. Jira takes care of graying the old ones

          I'll have a look at this issue tomorrow afternoon, pretty weird so far...

          Show
          jacques.le.roux Jacques Le Roux added a comment - BTW Tobias, a small hint, you don't need to rename your patches. Jira takes care of graying the old ones I'll have a look at this issue tomorrow afternoon, pretty weird so far...
          Hide
          mbrohl Michael Brohl added a comment -

          Tobias Laufkötter, I have reverted the commits because they introduced bug, see OFBIZ-9446.

          Show
          mbrohl Michael Brohl added a comment - Tobias Laufkötter , I have reverted the commits because they introduced bug, see OFBIZ-9446 .
          Hide
          tlaufkoetter Tobias Laufkötter added a comment -

          Jacques Le Roux, did you mean the automated unit and integration tests failed? Executing them with ./gradlew cleanAll loadAll test testIntegration does not throw an error with or without the latest patch.

          Inserting the originial content after filtering the list does the trick and solves the problem of OFBIZ-9446. The originial content will therefore not be filtered out if the fromDate and thruDate don't match, but I guess that's okay. It hasn't been filtered out before and since we don't know which kind of view we are getting, we don't know whether or not such dates are provided at all. Furthermore, the provided view supposedly is the root content anyway and was determined to be the correct one beforehand, so not filtering it by date again should not be an issue.

          (I didn't rename the patch file this time, let's see if this works )

          Show
          tlaufkoetter Tobias Laufkötter added a comment - Jacques Le Roux , did you mean the automated unit and integration tests failed? Executing them with ./gradlew cleanAll loadAll test testIntegration does not throw an error with or without the latest patch. Inserting the originial content after filtering the list does the trick and solves the problem of OFBIZ-9446 . The originial content will therefore not be filtered out if the fromDate and thruDate don't match, but I guess that's okay. It hasn't been filtered out before and since we don't know which kind of view we are getting, we don't know whether or not such dates are provided at all. Furthermore, the provided view supposedly is the root content anyway and was determined to be the correct one beforehand, so not filtering it by date again should not be an issue. (I didn't rename the patch file this time, let's see if this works )
          Hide
          mbrohl Michael Brohl added a comment -

          Thanks Tobias,

          your revised patch is in
          trunk r1800853
          release16.11 r1800854

          Show
          mbrohl Michael Brohl added a comment - Thanks Tobias, your revised patch is in trunk r1800853 release16.11 r1800854
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Tobias,

          Yep all works now, thank you Tobias for the details and Michael for the commit.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Tobias, Yep all works now, thank you Tobias for the details and Michael for the commit.

            People

            • Assignee:
              mbrohl Michael Brohl
              Reporter:
              tlaufkoetter Tobias Laufkötter
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development