Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Containers as files outside default host #17749

Closed
jgambarios opened this issue Dec 18, 2019 · 16 comments
Closed

Containers as files outside default host #17749

jgambarios opened this issue Dec 18, 2019 · 16 comments

Comments

@jgambarios
Copy link
Contributor

jgambarios commented Dec 18, 2019

Containers as files are not working properly outside default host.

  1. Create new site with a container as files
  2. Add new template
  3. Add a container as files (using the advance template you have more control on the parseContainer directive).

Any of these combinations (path without host, path with host, identifier) can be use with the same result.

#parseContainer('/application/containers/large-column(lg-1)/','1576693758718')
#parseContainer('65cf86f5-5686-4e9a-99db-e6cde1499687','1576693846053') 
#parseContainer('//test1.dotcms.com/application/containers/large-column(lg-1)/','1576705055598')
  1. Create a Page using the template
  2. Try to add and save content to that page
  3. No content is added

The code is not going to find a host in the path even if the parseContainer have it and when there is no host the code will try to find the container in the default host instead of current host (https://github.com/dotCMS/core/blob/master/dotCMS/src/main/java/com/dotmarketing/portlets/containers/business/FileAssetContainerUtil.java#L165-L172), which is wrong, and when that happen no content can be add to the page.

Things we saw:

  • The /v1/page/{pageId}/content (PageResource) is called with containers without a host even if the template is using the host in the parseContainer directive.
  • The render endpoint (http://localhost:8080/api/v1/page/render/folder1/page2?language_id=1) is removing the host before to send it to the FE:
{"entity":{
  ...
  "containers" : {
    ...
    "/application/containers/large-column/" : {
      "container" : {
        "identifier" : "a050073a-a31e-4aab-9307-86bfb248096a",
        "preLoop" : "#dotParse(\"//demo.dotcms.com/application/containers/large-column/preloop.vtl\")",
        "postLoop" : "#dotParse(\"//demo.dotcms.com/application/containers/large-column/postloop.vtl\")",
        "path" : "/application/containers/large-column/",
      }
    }
  },
  ...
  "template" : {
  ....
    "body" : "## Container: Large Column (lg-1)\r\n## This is autogenerated code that cannot be changed\r\n#parseContainer('//demo.dotcms.com/application/containers/large-column/','1576709049598')\r\n"
  }
  }
}

Possible solutions

  1. Make the render endpoint to handle better the hosts, if the parseContainer does not have it we could append the current host to it.
    If the parseContainer already has a host we should respect it.
  2. We could add a better default in https://github.com/dotCMS/core/blob/master/dotCMS/src/main/java/com/dotmarketing/portlets/containers/business/FileAssetContainerUtil.java#L165-L172 using the current host but that implies refactor in order to send the host down the flow until that method.

Acceptance Criteria

  1. We need to reproduce this case in integration tests before to fix it
@wezell wezell added this to the Bug Sprint milestone Jan 8, 2020
jdotcms added a commit that referenced this issue Jan 15, 2020
…ner is not part of the current host, and also make several fixes to make it work on the edit mode
jdotcms added a commit that referenced this issue Jan 16, 2020
jdotcms added a commit that referenced this issue Jan 16, 2020
jdotcms added a commit that referenced this issue Jan 16, 2020
jdotcms added a commit that referenced this issue Jan 18, 2020
…t when it is a container in the default host
jdotcms added a commit that referenced this issue Jan 18, 2020
…t when it is a container in the default host 2
jdotcms added a commit that referenced this issue Jan 21, 2020
@jdotcms
Copy link
Contributor

jdotcms commented Jan 23, 2020

PR

fmontes pushed a commit that referenced this issue Jan 27, 2020
* #17749 this first effort renders on the absolute path when the container is not part of the current host, and also make several fixes to make it work on the edit mode

* #17749 start adding unit test

* #17749 adding unit test for HostUtil

* #17749 adding fixes and unit test

* #17749 a new test case when the host does not exists, returns the default

* #17749 Adding fixes to be able to add an container on non-default host when it is a container in the default host

* #17749 Adding fixes to be able to add an container on non-default host when it is a container in the default host 2

* #17749 feedback done
@fmontes
Copy link
Member

fmontes commented Jan 27, 2020

Still not saving content.

@DeanGonzalez
Copy link

I would like to test this when there is a fix - I can help QA

@freddyucv
Copy link

PR: #17950

@dsilvam
Copy link
Contributor

dsilvam commented Feb 22, 2020

Leaving all code changes related to this card into 5.2.6. since after discussing with @freddyucv they are safe to leave.

@bryanboza
Copy link
Member

We will move the card to 5.2.7

@freddyucv
Copy link

PR: #18033

freddyucv pushed a commit that referenced this issue Feb 25, 2020
@freddyucv
Copy link

Note for QA:

freddyucv pushed a commit that referenced this issue Feb 25, 2020
freddyucv pushed a commit that referenced this issue Feb 25, 2020
freddyucv pushed a commit that referenced this issue Feb 25, 2020
freddyucv pushed a commit that referenced this issue Feb 25, 2020
freddyucv pushed a commit that referenced this issue Feb 26, 2020
freddyucv pushed a commit that referenced this issue Feb 26, 2020
freddyucv pushed a commit that referenced this issue Feb 26, 2020
dsilvam pushed a commit that referenced this issue Feb 26, 2020
* Revert "Sending the full container path to the UI (#17950)"

This reverts commit 655f786.

* Fixing containers error

* #17749 doc

* #17749 testing

* #17749 refactoring

* #17749 refactoring

* #17749 refactoring

* #17749 doc

* #17749 refactoring

* #17749 miss spelling
@dsilvam dsilvam added the Merged label Feb 26, 2020
@fmontes
Copy link
Member

fmontes commented Mar 17, 2020

How I tested:

  1. Create a site
  2. Create a file container in the site
  3. Create a template in this new site and add the container from this site and from demo
  4. Create a page and add, edit, remove content to this page

Also

In demo:

  1. Create a template
  2. Add containers from demo and from the other site
  3. Create a page
  4. Add, edit, remove content to this page

@bryanboza
Copy link
Member

Fixed, tested on master // Postgres // FF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests