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

Add three reports #14338

Merged
merged 17 commits into from Dec 2, 2021
Merged

Add three reports #14338

merged 17 commits into from Dec 2, 2021

Conversation

michebs
Copy link
Contributor

@michebs michebs commented Sep 14, 2021

This PR adds the following reports requested on this PM:

  • Top Users by likes received
  • Top Users by likes received from a user with a lower trust level
  • Top Users by likes received from a variety of people

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@jjaffeux
Copy link
Contributor

jjaffeux commented Sep 14, 2021 •
edited

@michebs there are linting errors can you have a look please?

@jjaffeux jjaffeux requested a review from Flink September 14, 2021 14:48
@michebs michebs closed this Sep 14, 2021
@michebs michebs reopened this Sep 14, 2021
@michebs
Copy link
Contributor Author

michebs commented Sep 14, 2021

@michebs there are linting errors can you have a look please?

done

Flink
Flink reviewed Sep 15, 2021
View reviewed changes
Copy link
Contributor

@Flink Flink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL queries look fine to me 👍
It would be best to have specs for these new reports IMO but I guess it’s your call @jjaffeux 😉

@SamSaffron
Copy link
Member

@techapj can you help @michebs with the tests here, we don't need anything radical, just to ensure the SQL is all working, so we catch regressions if we play with the SQL

@arpitjalan
Copy link
Member

@michebs Here is a test I wrote for 'top_users_by_likes_received' report: arpitjalan@d0aadfa

Following the above example you should be able to write tests for remaining two reports. Let me know if you need any further help here.

@arpitjalan
Copy link
Member

Did you had a chance to look into this @michebs?

@michebs
Copy link
Contributor Author

michebs commented Nov 29, 2021 •
edited

Did you had a chance to look into this @michebs?

@techapj Added the other two tests based on the test you did. Thanks

@michebs michebs requested a review from Flink November 29, 2021 01:59
@arpitjalan arpitjalan merged commit 9b5836a into discourse:main Dec 2, 2021
CvX added a commit that referenced this pull request Feb 1, 2022
@CvX
DEV: Remove install-imagemagick script
92af04a 
Seems to be accidentally added in #14338?
@CvX CvX mentioned this pull request Feb 1, 2022
Merged
CvX added a commit that referenced this pull request Feb 1, 2022
@CvX
DEV: Remove install-imagemagick script (#15768)
7ee7089 
Seems to be accidentally added in #14338?
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/trust-level-wishlist-items/141349/21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arpitjalan arpitjalan arpitjalan approved these changes

@jjaffeux jjaffeux Awaiting requested review from jjaffeux

@Flink Flink Awaiting requested review from Flink

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
7 participants
@michebs @CLAassistant @jjaffeux @SamSaffron @arpitjalan @discoursebot @Flink