-
Notifications
You must be signed in to change notification settings - Fork 120
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
Filtering based on average Review/Comment scores and Sorting #569
base: dev
Are you sure you want to change the base?
Conversation
Release 3.6.0
Release 3.7.0
Release 3.7.1
Hi guys, |
there is also an online demo here (ps1.7.7.7):
|
I will add more explanation to the Description or in comments ;) thanks |
]; | ||
|
||
$query = 'SELECT count(g.grade) as count ,g.grade FROM (SELECT t.id_product, case | ||
when t.avg_grade between 1 and 1.99 then "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be able to do 0.5 star? I think it costs nothing to start at 0 instead of 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually the average score can not be less than 1, because we do not have comments with 0 grade, user must choose between 1-5 values.
when t.avg_grade between 3 and 3.99 then "3" | ||
when t.avg_grade between 4 and 5 then "4" | ||
end as grade | ||
FROM (SELECT avg(h.grade) as avg_grade, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two much subqueries, I'm afraid of performance problems 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you are right, I think at least 3 joins are inevitable, but an alternative could be another table which keeps counts for all categories, and can be calculated during indexing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thank you @samberrry that's a good idea, but I think there are missing important things before being able to merge it.
You can't select two stars, which is something we should be able to do like "I want all 4 and 5 stars items"
Seen SQL request are buggy ^^
I'll continue the review after ;)
Hi @PierreRambaud, Thanks for review and your comments. so, I've implemented something like what Amazon does in filtering: we do not filter based on counts, so when we click on this: Also the number 3 in here: Again thanks for your review, I should fix the items you mentioned on code.. actually I could not work on the feature too much to optimized and review it very well, we still have some considerations in some sections which one of the important issues is about indexing. It works well when a moderator must approve comments, because I do indexing on comment validated action hook, we also need to do indexing on comment addition when the moderator checking is disabled. Thanks ;) |
I really like your idea and better understand the reason about the |
@PierreRambaud Thank you 😊 |
Hey @PierreRambaud, I was just working on comment indexing and I noticed that the product comment module is using doctrine ORM to store the product comment models, and there is not action hook to get comment addition event or delete event. We need it for the case that the moderator approval is disabled: |
We have two possible choices:
I'm not sure we should remove the compatibility range of this module since it's use by a lot of people, maybe playing with SQL should be enough |
yes. But how should we get notified on comment addition/deletion even? |
@samberrry @PierreRambaud Guys, this will be a NICE addition! Impressive 🚀 Few points from myself:
|
Hi, thanks for your interest 😉I agree with you on to keeping performance a priority. |
Release 3.8.0
Hey @samberrry It's a great contribution. I know you're probably busy, but will you continue with it? It's been a while since the last activity there. |
Co-authored-by: GoT <[email protected]>
Co-authored-by: GoT <[email protected]>
Co-authored-by: GoT <[email protected]>
Co-authored-by: GoT <[email protected]>
Co-authored-by: GoT <[email protected]>
Co-authored-by: GoT <[email protected]>
c247127
to
4f79cb8
Compare
|
@samberrry PRs must be submitted against |
Oh thanks! mistaken :) |
This change is