deanmoses / zenphoto-json-rest-api

Rest API for Zenphoto
GNU General Public License v2.0
9 stars 1 forks source link

Support some gallery statistics like latest images #8

Closed acrylian closed 7 years ago

acrylian commented 7 years ago

Some extra parameters for the gallery index page could be features like &latestimages=<number> if the image_album_statistics is available. I could imagine that this might be needed quite often.

deanmoses commented 7 years ago

The gallery-level stats are done and tested and documented in the README.

The album-level stats are still left to do.

The following commit, which finished the gallery-level stats, should have referenced this issue: https://github.com/deanmoses/zenphoto-json-rest-api/commit/9a8b6c366641b032b1e79c8ed8bc9437933f4188

acrylian commented 7 years ago

Great, I hope to find time to work on the Zenpage parts soon.

Should we really require the plugin? Perhaps it would be cleaner to check if it is enabled or its functions being available so backend settings are not bypassed. People might expect that, especially in case someone allows api access to third parties and does not want it at all. It could return an error message/notice. Not sure if 204 "no content" or just 404 or even 406 fits better.

IMHO we should do this with all plugins.

deanmoses commented 7 years ago

Are you asking should the JSON plugin require the Stats plugin? It doesn't! It returns a "Plugin not enabled" error for the stats JSON node, like this:

"stats":{"error":"Plugin not enabled: image_album_statistics"}

acrylian commented 7 years ago

No, I just stumbled upon line 466 where it is loaded via require_once?

deanmoses commented 7 years ago

It only does the require_once if the stats plugin is enabled. If the stats plugin is NOT enabled, it returns an error message instead:

if (!self::$statsPluginEnabled) {
    $errorMessage = gettext_pl('Plugin not enabled:  ', 'json_rest_api') . self::$statsPluginName;
    $ret['stats']['error'] = $errorMessage;
}
else {
    require_once(SERVERPATH . '/' . ZENFOLDER . '/' . PLUGIN_FOLDER . '/' . self::$statsPluginName . '.php');
}

I had to do the require_once because the stat plugin's getAlbumStatistic() getImageStatistic() weren't available until I did. Is there a better way?

acrylian commented 7 years ago

Ah, of course, my mistake. Image_album_statistics as a normal plugin that is loaded later. The order is CLASS_PLUGIN, FEATURE_PLUGIN and then normal non filter plugins. Probably there is indeed no other way.

deanmoses commented 7 years ago

Good to know it's not obvious. I updated the method's documentation to be clearer what's going on and why:

/**
 * Return true/false whether the stats plugin is enabled.  
 *
 * Additionally, if this is the FIRST time this method is called, either:
 * 1) require_once() the stats plugin to make its functions available, or
 * 2) add an error message to $ret saying that the stats plugin is disabled.
 *
 * @param array $ret the main JSON-ready array
 * @return boolean
 */
deanmoses commented 7 years ago

You can now get statistics at both the gallery and the album level.

Supports all the various options like count, sort order and depth of the stat plugin's getAlbumStatistic() function.

Look at README.md for full syntax. Here's a complicated example: http://mysite.com/myAlbum/?json&popular_albums=count:3,sort:asc,threshold:2&popular_images=count:2,deep:true

There are full tests. Closing this one.

acrylian commented 7 years ago

Just asking since I yet haven't had personal use case for the api: Do we need to be able to get popular albums and images in one request? URL queries like this look a bit better/cleaner to me and are more easily to process?: http://mysite.com/myAlbum/?json&popular_albums=<number to get>&sort=asc&threshold=2

deanmoses commented 7 years ago

I work with a lot of browser-based clients and have two rules of thumb around API design:

  1. For browser based clients, it generally results in faster page render times to make fewer network trips that return more data. This is particularly true with mobile networks because there's so much connection latency it's even more efficient to minimize the number of trips and increase payload size.
  2. Even when you think you can predict what people are going to need, you're probably wrong. So let them slice and dice the data in multiple ways.

In my case in particular I would like to display the latest album and the latest image on my front door, which is a case to get both at the same time.

deanmoses commented 7 years ago

I did, however, consider having an option to only get stats and not retrieve the gallery or album info at all. I see use cases for that, but since the URL structure is always requesting a specific resource, I don't see a semantically clean way to do it.

I think that if you get statistics when requesting the gallery without getting any albums adds very little time over the cost of collecting the statistics. Like this:

http://mysite.com/?json&level=0&popular_album

On my TODO list is to instrument the tests to confirm that. Then at least we can offer guidance for this particular scenario.

acrylian commented 7 years ago

Alright , thanks. I do understand and figured that might be the reason regarding slower mobile networks.

The index page probably would be the best way to get statistics without any album data. Zenphoto of course will get a list of albums on the index page anyway.