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

List all images associated to a document #751

Open
mgerbaux opened this issue Sep 24, 2019 · 10 comments
Open

List all images associated to a document #751

mgerbaux opened this issue Sep 24, 2019 · 10 comments
Assignees
Labels

Comments

@mgerbaux
Copy link

The list of all images associated to a document should be available to be displayed more practically than through the slideshow tool (especially for documents with large collection of images like https://www.camptocamp.org/articles/1058594/fr/concours-photo-sophie-2018)

@cbeauchesne
Copy link
Member

Links that should filters results (and don't) :

@asaunier
Copy link
Member

asaunier commented Oct 11, 2019

The list of all images associated to a document should be available to be displayed more practically than through the slideshow tool (especially for documents with large collection of images like https://www.camptocamp.org/articles/1058594/fr/concours-photo-sophie-2018)

This description is not very relevant: the API has no slideshow tool.
Do you mean that the API should provide the full list of images associated to a given document?
For instance https://api.camptocamp.org/images?c=1058594 but without the paging limits (default is 30)?

At least it is possible to add a limit=100 parameter to the URL above, eg. https://api.camptocamp.org/images?c=1058594&limit=100
even tough there is a limit to the limit (100 ?).

I don't think it is a bug: it's a wanted limit to avoid returning too many items at once and load the API too much.

@cbeauchesne
Copy link
Member

Agree that the bug classification is not so relevant, could be feature request.

Do you mean that the API should provide the full list of images associated to a given document

No, it must keep the limit/offset behavior, it's not a big deal for UI to implement a paged view, or an infinite scroll.

The issue is only about the /images?<document-type>=<document-id> entry point : it should returns images associated to <document-type>/<document-id>.

@asaunier
Copy link
Member

@cbeauchesne Thanks for clarifying but

The issue is only about the /images?= entry point : it should returns images associated to /.

  • Why is the document type relevant? Isn't the document_id sufficient?
  • Do you mean that https://api.camptocamp.org/images?c=1058594 for instance is not returning the correct set of images (ie. associated to article 1058594)?

I think this ticket is confusing and does not describe clearly the problem, with the current situation and the expected one. Shouldn't it be reformulated?

@cbeauchesne
Copy link
Member

cbeauchesne commented Oct 13, 2019

Do you mean that https://api.camptocamp.org/images?c=1058594 for instance is not returning the correct set of images (ie. associated to article 1058594)?

Yes? Actually, c=1058594 is simply ignored.

Isn't the document_id sufficient?

Technicly, document_id is unique, whatever the document type. It means that all entry point that requires document type could have been written by omitting document type. But as far as I know, only history entry point does not specify document type : /document/56065/history/fr So, yes, document_id could sufficient. But, IMO, it would be preferable to stick to other entry point, and specify document type (e.g. c for this issue).

Shouldn't it be reformulated?

Yes :)

@momomaniac
Copy link
Contributor

I manged to make this item work in #804
for the moment it is a test and only operational for
/images?o=xxx for outings
/images?c=xxx for articles

that questioned me on some points:

  • is this feature really supposed to work ? (@cbeauchesne you marked it as bug) I could not find the code which should do the work
  • has the syntax /images?c=xxx been decided / discussed somewhere
  • I do not really like the syntax, I find it ambiguous. I would prefer something more explicit like /images?associated_to_id=xxx which would also be a single query parameter for all document types since the document_id is unique as said by asaunier
  • shall it be possible to combine the query parameter with other search criteria, i.e date range, etc. ?

I would propose, I implement a simple solution with an identical query attribute for all document types (for example ?associated_to_id=xxx) and this attribute can be accesses form the routes for any document (/images, /routes, /articles, etc.)
Tell me what you think about the query parameter.

@cbeauchesne
Copy link
Member

is this feature really supposed to work ? (@cbeauchesne you marked it as bug) I could not find the code which should do the work

It was working on V5, but it never worked on V6. So yes, we can see this as a faeture :)

has the syntax /images?c=xxx been decided / discussed somewhere

It sticks to the rest of the api :

I do not really like the syntax, I find it ambiguous. I would prefer something more explicit like /images?associated_to_id=xxx which would also be a single query parameter for all document types since the document_id is unique as said by asaunier

I agree, there is a design flaw IMO with this id. It's unique among all the DB, but we always have to deal with document types. And worse, there is two typology of document types (waypoint VS w, routes vs r, article vs b ...).
But, it will be an entire refactoring project to support a simplier API interface, and if you crawl the code, you'll see that it'll simplify it a lot, with lot of factorization. But for now, it's better to stay coherent, even with a design flaw.

shall it be possible to combine the query parameter with other search criteria, i.e date range, etc. ?

Sure ! But don't be too greedy ;)

@momomaniac
Copy link
Contributor

momomaniac commented Apr 21, 2020 via email

@cbeauchesne
Copy link
Member

Actually, there is some entry point that use a common key, for instance : https://www.camptocamp.org/associations-history?d=1203070

But the point of using ES is to avoid access to DB. If the entry point is used a lot, it's better to use ES. But that's theory, as we do not have any visibility of actual charge, can't say if it's true for real 😞

Just, will it be not too complicated to handle both filtering : https://www.camptocamp.org/images?c=1058594&other_key_in_es=blabla ? If I understand well your message, you're saying /images entry point never use ES ?

@momomaniac
Copy link
Contributor

momomaniac commented Apr 23, 2020 via email

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

No branches or pull requests

4 participants