OneZoom / OZtree

OneZoom Tree of Life Explorer
Other
90 stars 20 forks source link

Sponsor choice of images not sticking #446

Closed hyanwong closed 2 years ago

hyanwong commented 2 years ago

When an alternative image is chosen on the sponsor_leaf page, the chosen src_id does not seem to appear in the user_preferred_image_src_id database field, and user_nondefault_image is left unset. We should somehow set up some unit testing for the sponsor_leaf pathway to avoid this, I think.

lentinj commented 2 years ago

@hyanwong What do you make of the above commit? There were various odd parts of the existing code. I've tried my best to tidy up but it would be good to see if I've missed some history of why things are. In particular this part seems very odd:

https://github.com/OneZoom/OZtree/commit/643f3d98ef495e93281caad22b1be95ad4736c54#diff-61551657463b2a1a1b966126bd5977300402382c9c7c7d913546eed79b0825e5L258-L263

...bearing in mind the default should already be in image_objects, it seems to be a separate attempt at pre-populating the initial default item that probably isn't ever triggered now?

It does seem to work though, with 0,1 and many images.

We should somehow set up some unit testing for the sponsor_leaf pathway to avoid this, I think.

Given the current logic is spread across template, javascript and javascript generated in javascript this is going to be tricky as-is. Ideally we'd try and generate a image selecting javascript "widget" that encapsulates the image-choosing. This could then be wrapped in tests, and the main form would just have to include it and tell it where the relevant hidden fields are.

hyanwong commented 2 years ago

It's very complicated isn't it. Any modularization/simplification would be great.

The main point is that (a) the current default OneZoom image should be the first one in the array (b) we should fill the rest of the array with images from the EoL API, if it is active (sometimes it is down. in which case we should fall back to the one that we have already) (c) if the default is already an EoL image we can remove the duplicate from the list.

If image_objects is already populated with the default from OneZoom then yes, it's probably a separate bodged attempt to do a-c above.

hyanwong commented 2 years ago

I think the code now works fine, thanks @lentinj. But the code that harvests the image, on validation, seems not to be working. It runs a script, called ./OZprivate/ServerScripts/Utilities/EoLQueryPicsNames.py, which does generally work, as I have been using it recently. So I can look at this (and will leave this issue open).

A good test case is life/@Dermatemys_mawii=98390?pop=osl_98390 when a better picture is given by EoL.

hyanwong commented 2 years ago

I think the code now works fine, thanks @lentinj. But the code that harvests the image, on validation, seems not to be working. It runs a script, called ./OZprivate/ServerScripts/Utilities/EoLQueryPicsNames.py, which does generally work, as I have been using it recently. So I can look at this (and will leave this issue open).

A good test case is life/@Dermatemys_mawii=98390?pop=osl_98390 when a better picture is given by EoL.

Fixed and tested in https://github.com/OneZoom/OZtree/commit/24cfb17d94f36f73f2c527175ca0e1fa3cbb6d51

hyanwong commented 2 years ago

Although this works, I wonder if it is worth leaving open to address the comments above:

Ideally we'd try and generate a image selecting javascript "widget" that encapsulates the image-choosing. This could then be wrapped in tests, and the main form would just have to include it and tell it where the relevant hidden fields are.

There's a lot of intricacy in this code and packaging it up more sensibly, with an extra pair of eyes on it and some tests, might be quite useful.

hyanwong commented 2 years ago

Closed in favour of #516