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

Allow Image field to be used as the contentlet.titleImage #15575

Closed
wezell opened this issue Nov 20, 2018 · 11 comments

Comments

@wezell
Copy link
Contributor

commented Nov 20, 2018

in #15473 we added the property of titleImage to a contentlet that will send the results of the first binary field with an image on any contentlet.
We need to also enable scanning of Image Fields - so the rule will be :

  1. return the first ImageField or BinaryField that has an image in it.
  2. Title Image needs a specific endpoint meaning, we need to append titleImage to get the title image e.g. /dA/1212-123/titleImage
  3. If no title image is found, we need to return a 404
  4. This needs tests.
  5. Remove the title image from the ESMapping - not needed there

@wezell wezell added this to the Rex Current milestone Nov 20, 2018

@dsilvam dsilvam modified the milestones: 11272018_REX, Rex Current Nov 27, 2018

@wezell wezell modified the milestones: Rex Current, Cody Current Dec 6, 2018

wezell added a commit that referenced this issue Jan 4, 2019

wezell added a commit that referenced this issue Jan 4, 2019

wezell added a commit that referenced this issue Jan 4, 2019

dsilvam added a commit that referenced this issue Jan 4, 2019

#15575 added hovers and selector (#15772)
* #15575 added hovers and selector

* #15575 cleanup

* #15575 removed alert
@dsilvam

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

PR: #15772

@dsilvam dsilvam added the WF : Merged label Jan 4, 2019

@wezell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

To test the latest changes that force explicitly adding titleImage to the call:

http://127.0.0.1:8080/dA/2943b5eb9105 == 404
http://127.0.0.1:8080/dA/2943b5eb9105/titleImage == image

wezell added a commit that referenced this issue Jan 7, 2019

wezell added a commit that referenced this issue Jan 7, 2019

@fabrizzio-dotCMS fabrizzio-dotCMS self-assigned this Jan 8, 2019

@fabrizzio-dotCMS

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

In this example, I always get theback image even when I hit both URLs
http://127.0.0.1:8080/dA/2943b5eb9105 == image
http://127.0.0.1:8080/dA/2943b5eb9105/titleImage == image

Even if I do this
http://127.0.0.1:8080/dA/2943b5eb9105/titleImageLOL
I still get an Image

Here an example that returns an image out of a binary field
http://127.0.0.1:8080/dA/eae8b6e478
and here how I know this is a binary field
http://localhost:8080/api/v1/content/versions/?identifier=eae8b6e4-7824-42e4-81dc-51b95cc39540

This one does not have an ImageTitle
http://localhost:8080/api/v1/content/versions/?identifier=a9f30020-54ef-494e-92ed-645e757171c2
For which the servlet returns a 404
http://127.0.0.1:8080/dA/a9f3002054/titleImage

@wezell wezell assigned wezell and unassigned fabrizzio-dotCMS Jan 8, 2019

wezell added a commit that referenced this issue Jan 8, 2019

wezell added a commit that referenced this issue Jan 9, 2019

wezell added a commit that referenced this issue Jan 9, 2019

wezell added a commit that referenced this issue Jan 10, 2019

wezell added a commit that referenced this issue Jan 10, 2019

wezell added a commit that referenced this issue Jan 10, 2019

@bryanboza bryanboza added this to CODY in QA Jan 10, 2019

@dsilvam

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

PR: #15808

wezell added a commit that referenced this issue Jan 10, 2019

@jgambarios

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

We still have issues with the multilanguage support:

  1. Create a spanish version of any News content.
  2. In the spanish version use a different image than the English version

Works:

http://localhost:8080/dA/2943b5eb-9105-4dcf-a1c7-87a9d4dc92a6?language_id=1

Does not work (404):

http://localhost:8080/dA/2943b5eb-9105-4dcf-a1c7-87a9d4dc92a6?language_id=2

@jgambarios jgambarios assigned jdotcms and unassigned wezell Jan 24, 2019

@jgambarios

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

PR: #15894

@bryanboza bryanboza added this to CODY in QA Jan 28, 2019

@jgambarios jgambarios removed this from CODY in QA Feb 1, 2019

@jgambarios

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

There is a null pointer exception when:

  1. Create a working content in spanish language only
  2. Request it using the default language languageId = 1

The problem can be fix adding the proper returns in lines:

https://github.com/dotCMS/core/blob/master/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java#L365
https://github.com/dotCMS/core/blob/master/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java#L374

NOTE: Before to add the fix an integration test should be write in order to reproduce the bug.

jdotcms added a commit that referenced this issue Feb 4, 2019

@jgambarios

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

PR Integration test: #15959

@bryanboza

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Fixed, tested with the attached comments and works as expected

@bryanboza bryanboza moved this from CODY to Done in QA Feb 25, 2019

@wezell wezell closed this Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.