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

Spam-report for juice-shop pull requests and issues #29

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hxrshxz
Copy link

@hxrshxz hxrshxz commented Feb 21, 2025

Description

Spam issues and Pull requests report graph for the Juice-shop app

Screencast.from.2025-02-22.03-06-25.mp4

Copy link
Member

@bkimminich bkimminich left a comment

Choose a reason for hiding this comment

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

Retrieving the last 30 days every time is not the way to go. Take a look at the other GitHub stats, we pull the data of the previous day and append it to a JSON, then commit to the repo. This means we have a growing permanent history. For the initial JSON you'd need to run the script for each day starting with the first day that ever had a spam PR/issue, then concatenate all subsequent days except the day that would be appended by the next CI run.

Also, the visualization is missing, and without the PR is too incomplete to merge.

Lastly, please remove your GitHub token. I think it works without - see the other GitHub stats again: https://github.com/juice-shop/juicy-statistics/blob/master/extractors/github.js

@hxrshxz
Copy link
Author

hxrshxz commented Feb 23, 2025

Retrieving the last 30 days every time is not the way to go. Take a look at the other GitHub stats, we pull the data of the previous day and append it to a JSON, then commit to the repo. This means we have a growing permanent history. For the initial JSON you'd need to run the script for each day starting with the first day that ever had a spam PR/issue, then concatenate all subsequent days except the day that would be appended by the next CI run.

Also, the visualization is missing, and without the PR is too incomplete to merge.

Lastly, please remove your GitHub token. I think it works without - see the other GitHub stats again: https://github.com/juice-shop/juicy-statistics/blob/master/extractors/github.js

I will be occupied with my university exams until March 7th 😅 . After that I will go through the codebase to understand it better and implement a similar approach for the spam-report and make the necessary changes

@hxrshxz hxrshxz marked this pull request as draft February 23, 2025 13:10
@hxrshxz
Copy link
Author

hxrshxz commented Mar 8, 2025

I am almost done with tihs , just need to figure out some stuff then i will push the changes

@hxrshxz hxrshxz reopened this Mar 10, 2025
@hxrshxz hxrshxz marked this pull request as ready for review March 10, 2025 22:24
@hxrshxz hxrshxz requested a review from bkimminich March 10, 2025 22:24
@hxrshxz
Copy link
Author

hxrshxz commented Mar 10, 2025

For the initial JSON you'd need to run the script for each day starting with the first day that ever had a spam PR/issue,then concatenate all subsequent days except the day that would be appended by the next CI run.

Populated the spam-report.json with a script to fetch the no.of spam pr/issue starting from the 1st ever spam pr/issue

then concatenate all subsequent days except the day that would be appended by the next CI run.

added a similar function to achieve this after going throught the approach used in github.js.

Also, the visualization is missing, and without the PR is too incomplete to merge.

added two graphs (1st one is the default what we are using and another alternative(chart.js)) since the google chart's one doesn't seem as promising. Take a look at both and let me know which one we should proceed with. I will further improve the chosen graph to make it even better if possible

1st Graph

Screencast.from.2025-03-11.04-14-32.mp4

2nd Graph (chart.js)

Screencast.from.2025-03-11.04-04-55.mp4

@J12934
Copy link
Member

J12934 commented Mar 12, 2025

sorry for chiming in there late.
just looking at the graphs, i think daily is way to fine as a granularity.
would be more interesting for me to see the monthly numbers to really understand how this has changed over time.
with the current graph this is pretty hard to tell how much this changed.

probably still fine to collect and store it on a daily basis, but wouldn't display it this way.

@bkimminich
Copy link
Member

Collecting daily, displaying monthly sum sounds best, agreed.

@hxrshxz hxrshxz marked this pull request as draft March 17, 2025 07:57
@hxrshxz hxrshxz marked this pull request as ready for review March 17, 2025 10:23
Signed-off-by: JuiceShopBot <[email protected]>
Copy link
Member

@J12934 J12934 left a comment

Choose a reason for hiding this comment

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

Screenshot 2025-03-17 at 20 15 56

For me the chart is taking up the entire screen width and height, which is really overwhelming, and doesn't really fit with the other charts. would limit the height to be more managable.

@@ -8,15 +8,16 @@
crossorigin="anonymous"></script>
<title>OWASP Juice Shop - Open Source Statistics</title>

<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason this is pulling in another chart libary?
seems like this graph should also be achievable with the already used google visualization lib, don't want to pull in two libs unless we have a good reason for it

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

Successfully merging this pull request may close these issues.

4 participants