Uploaded image for project: 'Bloomreach Forge'
  1. Bloomreach Forge
  2. FORGE-340

[External Document Picker] ExternalTreeItemFieldBrowserDialog should resolve expandable ancestor tree nodes from existing tree node data

Details

    Description

      At the moment, ExternalTreeItemFieldBrowserDialog#expandPickedExternalTreeItemNodes() depends on ExternalDocumentTreeService#getParent() method invocation to resolve the parent of the specific tree node item, when it needs to expand all the ancestor node of the selected tree node:

      ExternalTreeItemFieldBrowserDialog#expandPickedExternalTreeItemNodes()
           private void expandPickedExternalTreeItemNodes(ExternalDocumentServiceFacade<Serializable> extDocService) {
               for (Serializable item : getPickedExternalDocuments()) {
                   Serializable parent = extDocService.getParent(item);
                   while (parent != null) {
                       treeExpansionSet.add(parent);
                       parent = getExternalDocumentServiceFacade().getParent(parent);
                  }
               }
           }
      

      This is not convenient in reality because:

      • When opening the document in cms editor, it invokes #getFieldExternalDocuments() to get the data of the current selected items. However, in many real use cases, the backend api call may just return only the single item, not including parent-child relationship for the single item query (in REST api for example).

      Also, when looking at the ExternalTreeItemFieldBrowserDialog#expandPickedExternalTreeItemNodes() implementation, I've just realized that it is only about expanding the existing tree nodes of ancestors of the selected tree node item and so it does not have to retrieve the parent from the facade of the backend at all at this moment.

      An improvement would be to resolve the parent item data from the existing (UI) tree provider itself as it already exists there, instead of invoking facade#getParent() again.

      Attachments

        Activity

          wko Woonsan Ko (Inactive) added a comment - Fix provided in https://github.com/bloomreach-forge/external-document-picker/pull/5 .

          Hi clientserviceteam,

          Would you mind reviewing the PR?
          Thanks,
          Woonsan

          wko Woonsan Ko (Inactive) added a comment - Hi clientserviceteam , Would you mind reviewing the PR? Thanks, Woonsan

          Hi Woonsan,
          The change makes sense to me, and works in the demo.

          Shouldn't the now unused extDocService argument be removed from private expandPickedExternalTreeItemNodes method?
          Also, ExternalTreeItemDataProvider#getParent seems to delegate to the facade anyway?

          Cheers, Jeroen

          jhoffman Jeroen Hoffman added a comment - Hi Woonsan, The change makes sense to me, and works in the demo. Shouldn't the now unused extDocService argument be removed from private expandPickedExternalTreeItemNodes method? Also, ExternalTreeItemDataProvider#getParent seems to delegate to the facade anyway? Cheers, Jeroen
          wko Woonsan Ko (Inactive) added a comment - - edited

          Hi jhoffman,

          You're right! I will try to figure out a different, real solution than the current broken one. I've closed the PR as it's not a solution.

          Regards,

          Woonsan

          wko Woonsan Ko (Inactive) added a comment - - edited Hi jhoffman , You're right! I will try to figure out a different, real solution than the current broken one. I've closed the PR as it's not a solution. Regards, Woonsan

          It seems difficult to resolve parent-child relationship through Wicket tree - tableTree.
          So, I guess this can still be done in the facade level by implementing #getParent() properly.

          wko Woonsan Ko (Inactive) added a comment - It seems difficult to resolve parent-child relationship through Wicket tree - tableTree. So, I guess this can still be done in the facade level by implementing #getParent() properly.

          People

            Unassigned Unassigned
            wko Woonsan Ko (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: