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

Fix broken default logo image #3657

Merged
merged 4 commits into from
Jul 18, 2017

Conversation

klikstermkd
Copy link
Contributor

Fixes #3656

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Aleksandar Jovanov added 2 commits June 30, 2017 19:58
If you go to the sysadmin config page, and if you don't change the logo image and just click Update, the logo image will break.
The reason is because its been given wrong path to the image.
@klikstermkd
Copy link
Contributor Author

Ready for review.

@amercader amercader self-assigned this Jul 4, 2017
for f in os.listdir(ckan_images_path):
if f == value:
image_in_ckan = True
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.isfile can be used instead of a for loop here

@klikstermkd
Copy link
Contributor Author

@wardi Now it should be good.

@klikstermkd
Copy link
Contributor Author

@wardi Can you check this again? :)

@amercader
Copy link
Member

I think the underlying problem is that the admin form is receiving the correct value (/base/images/ckan-logo.png), even if the widget just displays the file name:

<input id="field-image-url" type="url" name="ckan.site_logo" value="/base/images/ckan-logo.png" placeholder="http://example.com/my-image.jpg" class="form-control" readonly="">

But sending the wrong value to the controllers, it will only send the file name (ckan-logo.png). Rather than adapt the action to this, it would be best to ensure that the form is sending data unchanged.

@klikstermkd
Copy link
Contributor Author

That still won't resolve the issue because the controller doesn't know whether the default image is uploaded from a user or available already from CKAN's local images directory.

@amercader
Copy link
Member

True, but then it's just a matter of covering the local image case in the config_option_update() check:

if key =='ckan.site_logo' and value and not (value.startswith('http') or value.startswith('/')):

@klikstermkd
Copy link
Contributor Author

Uploaded images from user and CKAN's images from local folder will pass the condition.

@amercader
Copy link
Member

Sorry, I meant:

if key =='ckan.site_logo' and value and not value.startswith('http') and not value.startswith('/'):

@klikstermkd
Copy link
Contributor Author

@amercader Can you check now?

@amercader amercader merged commit 8d08d3a into ckan:master Jul 18, 2017
klikstermkd pushed a commit to klikstermkd/ckan that referenced this pull request Jul 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default logo image not properly saved
5 participants