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.

Sign up for GitHub

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

Jump to bottom

unexpected result from elgg_list_entities_from_annotation_calculation (Trac #4393) #4393

Closed
elgg-gitbot opened this issue Feb 16, 2013 · 14 comments
Closed

unexpected result from elgg_list_entities_from_annotation_calculation (Trac #4393) #4393

elgg-gitbot opened this issue Feb 16, 2013 · 14 comments
Assignees
@hypeJunction
Labels
bug engine important
Milestone
Elgg 1.8.14

Comments

@elgg-gitbot
Copy link

Original ticket http://trac.elgg.org/ticket/4393 on 42166844-10-28 by trac user coldtrick, assigned to unknown.

Elgg version: 1.8

use case:
i log every view of a blog in an annotation. I added a tab on blogs to show the popular (most viewed) blogs.

i've setup my options as:

$options = array(
  "type" => "object",
  "subtype" => "blog",
  "annotation_names" => array("view"),
  "annotation_created_time_lower" => (time() - (30 * 24 * 60 * 60)),
  "calculation" => "count",
  "full_view" => false
);

this will result in the 10 most viewed blog in the past 30 days.

Now the problem:
If i use elgg_get_entities_from_annotation_calculation i get the object i requested, for example 2 blogs.

If i use elgg_list_entities_from_annotation_calculation i still get a list of the same 2 blog, but (depending on the number of views these blogs have) i get pagination.

This is because if you tell elgg_get_entities_from_annotation_calculation to count ($options["count"] = true) it counts the number of annotations, not the number of entities with the annotation. This is done in elgg_get_metastring_based_objects()
@elgg-gitbot
Copy link
Author

ewinslow wrote on 42365480-04-07

I'm confused about what the problem is. Your description sounds like the appropriate behavior.

@elgg-gitbot
Copy link
Author

brettp wrote on 42366179-06-12

This is a documentation problem. It needs to be clearer what count means in this context.

@elgg-gitbot
Copy link
Author

cash wrote on 42373059-11-23

I would expect it to count the entities, not the annotations.

@elgg-gitbot
Copy link
Author

trac user coldtrick wrote on 42374723-03-17

cash: +1

The problem is it counts the annotations not the entities. thus with only two blogs (which have more then 10 pageviews) I get pagination :(

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.8.7 by cash on 42464321-03-19

@elgg-gitbot
Copy link
Author

cash wrote on 42479763-10-08

There is a more fundamental problem with the annotation code. The query generated looks like this:

SELECT DISTINCT n_table.*, e.*, count(cast(a_msv.string as signed)) as annotation_calculation FROM elgg_annotations n_table 
JOIN elgg_metastrings a_msv ON n_table.value_id = a_msv.id  
JOIN elgg_entities e ON n_table.entity_guid = e.guid  
JOIN elgg_metastrings msn on n_table.name_id = msn.id  

WHERE (snip) 
GROUP BY n_table.entity_guid ORDER BY annotation_calculation desc, n_table.id LIMIT 0, 10
  1. It has both a group by and a distinct
  2. There is a secondary sort by annotation id
  3. It pulls back the entity and an annotation together which means the fields that are common between the two tables get overwritten. The entity table is second so I think its values take precedent.

The problem with elgg_list_entities_from_annotation_calculation() is that it uses elgg_get_entities_from_annotation_calculation() to do a count. The annotation code supports a shortcut of passing 'count' for an annotation count. If I turn off the shortcut, the count gets ignored because elgg_get_metastring_based_objects() does not support counting entities.

If the metadata/annotation code documentation says nothing about supporting 'count' for counting annotations/metadata, then I may be able to remove that shortcut and add code to support counting entities.

Longer term, we should think about improving this code.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42732055-09-20

Is this still high priority, or can it go in a later release? Sounds like Cash has one solution, is this implementable anytime soon?

@elgg-gitbot
Copy link
Author

cash wrote on 42737441-08-08

It can go in a later release. The solution wasn't that clean.

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.8.10 by trac user mrclay on 42883217-10-09

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.8.12 by brettp on 42934480-07-19

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.8.13 by brettp on 43014040-03-22

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.8.14 by cash on 43074389-05-22

cash added a commit to cash/Elgg that referenced this issue Mar 6, 2013
@cash
Fixes Elgg#4393 working around the overloaded 'count' parameter
2dbb977
@cash cash closed this as completed Mar 6, 2013
@cash cash mentioned this issue May 31, 2013
Closed
@hypeJunction
Copy link
Contributor

@cash Could you please take look? I think this fixes the problem. I have added more tests with the same PR

@cash
Copy link
Contributor

cash commented Oct 27, 2014

redirecting to @Elgg/owners since I'm not an active Elgg developer

mrclay pushed a commit to mrclay/Elgg-leaf that referenced this issue Dec 19, 2014
@hypeJunction @mrclay
fix(annotations): do not enforce callback in egefac()
1dcea2d 
Allow custom or no callback in Annotations::getEntitiesFromCalculation()

Fixes Elgg#7398

fix(metastrings): cast metastring calculation to an integer

egefac() with count => true was returning a string, which was not in line with the documentation and general ege* pattern

fix(annotations): fixes egefac() count shortcut

Passing count => true to egefac() makes ege* logic identitical to a normal elgg_get_entities_from_annotations().

Fixes Elgg#4393

chore(tests): improve egefac() test coverage

Adds more egefac() tests and cleans up some of the lint
@hypeJunction hypeJunction mentioned this issue Jan 14, 2015
Closed
mrclay pushed a commit to mrclay/Elgg-leaf that referenced this issue Jan 19, 2015
@hypeJunction @mrclay
fix(annotations): simplifies ege* for annotation calculations
347db4d 
egefac() with count => true was returning a string, which was not in line
with the documentation and general ege* pattern. Passing count => true to
egefac() now makes ege* logic identical to a normal
elgg_get_entities_from_annotations().

Fixes Elgg#7398, Elgg#4393
mrclay pushed a commit to mrclay/Elgg-leaf that referenced this issue Jan 19, 2015
@hypeJunction @mrclay
fix(annotations): simplifies ege* for annotation calculations
9742e29 
Annotations::getEntitiesFromCalculation() now allows custom or no callback.
Metastring calculations returned by ege* are now cast as integer. Passing
count => true to elgg_get_entities_from_annotation_calculation() now uses
elgg_get_entities_from_annotations() (this removes the magic and flags
used previously). Adds more egefac() tests and cleans up some of the lint.

Fixes Elgg#7398, Elgg#4393
@hypeJunction hypeJunction mentioned this issue Feb 18, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hypeJunction hypeJunction

Labels
bug engine important
Milestone
Elgg 1.8.14
Development

No branches or pull requests
3 participants
@cash @hypeJunction @elgg-gitbot