Providing score trend on plugins enhancement #495

AayushSaini101 · 2024-03-22T07:59:51Z

Description

Add a new field in the HTTP API that helps to analyze the evolution of the plugin's score.

Task :

Add extra field in scoreDetails that contains the previous score of the plugin.

Closes #493.

Testing Done

Added new test cases to verify the previous score of a plugin.
Improved the current test cases also according to the new update in this PR.

Submitter checklist

Give feedback

If an issue exists, it is well described and linked in the description

If an issue exists, it is well described and linked in the description
Options
Successfully updated the issue's project

There was an error updating the issue's project
The description of this pull request is detailed and explain why this pull request is needed

The description of this pull request is detailed and explain why this pull request is needed
Options
Successfully updated the issue's project

There was an error updating the issue's project
The changeset is on a specific branch. Using `feature/` for new feature, or improvements ; Using `fix/` for bug fixes ; Using `docs/` for any documentation changes.

The changeset is on a specific branch. Using feature/ for new feature, or improvements ; Using fix/ for bug fixes ; Using docs/ for any documentation changes.
Options
Successfully updated the issue's project

There was an error updating the issue's project
If required, the documentation has been updated

If required, the documentation has been updated
Options
Successfully updated the issue's project

There was an error updating the issue's project
There is automated tests to cover the code change / addition or an explanation why there is no tests in the description.

There is automated tests to cover the code change / addition or an explanation why there is no tests in the description.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

jmMeessen · 2024-03-22T09:35:15Z

Hello @AayushSaini101

Thank you for starting this Draft PR. Don't forget to explain in the description the "what", the "why", the "what-for", etc. Take the time to explain the PR to the maintainer in details.

Another useful thing to do before pushing a commit/pr is to verify that it builds locally. Obviously the checkstyle fails. Or if using TDD and first pushing a failing test, explain it.

alecharp · 2024-03-22T10:10:36Z

Please note, again, that there is a pull request template which is here to help you format what you contribute.

AayushSaini101 · 2024-03-25T07:45:57Z

Hello @AayushSaini101

Thank you for starting this Draft PR. Don't forget to explain in the description the "what", the "why", the "what-for", etc. Take the time to explain the PR to the maintainer in details.

Another useful thing to do before pushing a commit/pr is to verify that it builds locally. Obviously the checkstyle fails. Or if using TDD and first pushing a failing test, explain it.

Thanks @jmMeessen :) Done

alecharp · 2024-03-25T08:04:11Z

I fixed the description.

Please, the "previous score" shouldn't be in the "plugin". You are not starting correctly.

AayushSaini101 · 2024-03-25T08:12:17Z

#493

I fixed the description.

Please, the "previous score" shouldn't be in the "plugin". You are not starting correctly.

@alecharp My approach is to store the change of the score in a field of the score table, Isn't correct ?

AayushSaini101 · 2024-03-25T08:29:40Z

I will use that value [-1,0,1] means score change, score don't change, score increased that is stored in the column of the table, and according to the value present for individual plugin, we can show the message whether the score is decreased or not. what you think @alecharp

alecharp · 2024-03-25T09:07:09Z

We already keep the last 5 scores in the scores table. Don't duplicate entries like that.

AayushSaini101 · 2024-03-25T09:46:40Z

We already keep the last 5 scores in the scores table. Don't duplicate entries like that.

@alecharp Thanks, i checked the table but cannot able to find the last score list of a plugin, can you help where it is present,

alecharp · 2024-03-25T09:56:32Z

If you run the scoring multiple times, you will see that you have multiple entries for the same plugin_id.

AayushSaini101 · 2024-03-25T10:05:26Z

If you run the scoring multiple times, you will see that you have multiple entries for the same plugin_id.

Thanks, Yes there are multiple entries of a plugin, So the maximum duplicates of a single plugin_id can be 5.
In that case, I guess we can create an API that checks group the same plugin_id and check the behavior of the score whether it is increasing or decreasing. I guess the last 2 entries of a plugin_id are suitable to analyse the behaviour of a plugin scoring, What do you think ? @alecharp

alecharp · 2024-03-25T10:30:54Z

Rather than just knowning if it increases or decreases, I think it's preferable to know the score. This way, the UI can decide if its shows the value or if it shows a sign for the trend.

This would have been a great discussion on the issue, to understand the prerequisite of the work to be done, rather than here on a pull request.

AayushSaini101 · 2024-03-25T10:39:26Z

Rather than just knowning if it increases or decreases, I think it's preferable to know the score. This way, the UI can decide if its shows the value or if it shows a sign for the trend.

This would have been a great discussion on the issue, to understand the prerequisite of the work to be done, rather than here on a pull request.

Sure @alecharp I am moving the discussion on the issue

AayushSaini101 · 2024-03-28T09:02:11Z

Testing changes only for verification@alecharp I tried to add previous from one approach,

I am making single db call, but the score will contains at max 2 duplicate score records, this will change the ScoreStatistics values, Is it good ? Or we can make separate db call for the requirement

alecharp

You forced push this pull request, again.
Now we can see changes that were done in the main branch as part of this pull request.
Lots of files were changed that are not related to this pull request.

Statistics should not be changed because you creates. They should only be about the latest scores.

alecharp · 2024-03-28T09:57:24Z

.DS_Store

Shouldn't be here.

Resolved @alecharp

alecharp · 2024-03-28T09:57:35Z

Jenkinsfile

Resolved @alecharp

alecharp · 2024-03-28T09:57:59Z

core/src/main/java/io/jenkins/pluginhealth/scoring/model/Plugin.java

Resolved @alecharp

alecharp · 2024-03-28T09:58:10Z

core/src/main/java/io/jenkins/pluginhealth/scoring/model/Score.java

Resolved @alecharp

alecharp · 2024-03-28T09:59:12Z

...rc/test/java/io/jenkins/pluginhealth/scoring/probes/KnownSecurityVulnerabilityProbeTest.java

Resolved @alecharp

alecharp · 2024-03-28T09:59:36Z

war/.yarnrc.yml

Resolved @alecharp

alecharp · 2024-03-28T09:59:53Z

war/src/main/java/io/jenkins/pluginhealth/scoring/config/SecurityConfiguration.java

Resolved @alecharp

alecharp · 2024-03-28T10:00:31Z

war/yarn.lock

Resolved @alecharp

AayushSaini101 · 2024-03-28T10:16:47Z

You forced push this pull request, again. Now we can see changes that were done in the main branch as part of this pull request. Lots of files were changed that are not related to this pull request.

Statistics should not be changed because you creates. They should only be about the latest scores.

@alecharp Sorry these are not completed changes, i was just checking whether the approaches and changes can be used or not. This will not happen again :)

One Question; can we make two db call in this case? one for latest score and one for previous score in the same endpoint

AayushSaini101 · 2024-03-30T10:04:42Z

Here is the previous score for every plugin

alecharp

Please provide a test using different score and previous score for at least 2 plugins because the current test is not testing that.

AayushSaini101 · 2024-04-02T15:57:33Z

Please provide a test using different score and previous score for at least 2 plugins because the current test is not testing that.

Added new test case for verifying the previousScore @alecharp

alecharp

I don't understand why the getLatestScoresSummaryMap and getPreviousScoreMap method need a list of Score in input.

alecharp · 2024-04-03T13:37:07Z

core/src/main/java/io/jenkins/pluginhealth/scoring/service/ScoreService.java

-    public Map<String, Score> getLatestScoresSummaryMap() {
-        return repository.findLatestScoreForAllPlugins().stream()
-            .collect(Collectors.toMap(
-                score -> score.getPlugin().getName(),
-                score -> score
-            ));
+    public Map<String, Score> getLatestScoresSummaryMap(List<Score> scores) {
+        return scores.stream()
+                .collect(Collectors.toMap(
+                        score -> score.getPlugin().getName(),
+                        score -> score,
+                        (existingScore, newScore) -> existingScore));
+    }
+
+    @Transactional(readOnly = true)
+    public Map<String, Long> getPreviousScoreMap(List<Score> scores) {
+        return scores.stream()
+                .collect(Collectors.toMap(
+                        score -> score.getPlugin().getName(), // Key mapper
+                        Score::getValue, // Value mapper
+                        (existingValue, newValue) -> newValue // Merge function
+                        ));


I don't understand why the methods have a parameter.
Those methods don't use the database at all.

@alecharp I created this findLastTwoScoreForAllPlugins method to fetch last two score for all plugins, and these methods getLatestScoresSummaryMap and getPreviousScoreMap are used to find the last and last second score of a plugin, These methods takes the output generated by indLastTwoScoreForAllPlugins() method. @Transcation annotation need to removed i checked now thanks

alecharp · 2024-04-03T13:39:05Z

core/src/test/java/io/jenkins/pluginhealth/scoring/service/ScoreServiceIT.java

+        List<Score> scoreList = new ArrayList<>();
+        scoreList.add(p1s);
+        scoreList.add(p2s);
+        scoreList.add(p2sOld);
+        scoreList.add(p1sOld);
+        scoreList.add(p1sOld2);
+
+        final Map<String, Score> summary = scoreService.getLatestScoresSummaryMap(scoreList);


This shows that we need to provide the scores we want to see to the method, when those scores are in fact in the database.
The method makes no sense to me now.

Thanks @alecharp updated

alecharp · 2024-04-03T13:40:33Z

war/src/test/java/io/jenkins/pluginhealth/scoring/http/ScoreAPITest.java

+                        100, 1, List.of("There is no active security advisory for the plugin."))),
+                1));
+
+        final ZonedDateTime latestTimePlugin2 = ZonedDateTime.now().minusMinutes(2);


never used?

@alecharp removed

alecharp · 2024-04-03T13:41:26Z

war/src/test/java/io/jenkins/pluginhealth/scoring/http/ScoreAPITest.java

+                                                        scoreP1Date.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME),
+                                                        scoreP1Date.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME)),
+                                        false));
+    }


reactivate the formatter before closing the method.

@alecharp Cannot format manually once I edit the file, spotless causing build failure

you need to add // @formatter:on before the end of the method.

Done @alecharp

AayushSaini101 · 2024-04-24T04:53:05Z

@alecharp I guess, i have answered and resolved all your comments

alecharp · 2024-04-24T07:05:56Z

I don't know I have to check. Please see to fill the pull request description first.

AayushSaini101 · 2024-05-03T06:13:54Z

I don't know I have to check. Please see to fill the pull request description first.

Done @alecharp

alecharp · 2024-05-13T08:06:34Z

There is still a TODO in the pull request description @AayushSaini101

AayushSaini101 · 2024-05-13T08:16:00Z

There is still a TODO in the pull request description @AayushSaini101

Added more points @alecharp thanks

alecharp · 2024-05-13T08:32:11Z

And still, there is a TODO

alecharp · 2024-05-13T09:03:07Z

In fact, no. It's important to add the testing that you have done for this pull request, this is why it's in the pull request template. Removing that section is not the way to go, it needs to be filled.

AayushSaini101 · 2024-05-13T09:16:39Z

In fact, no. It's important to add the testing that you have done for this pull request, this is why it's in the pull request template. Removing that section is not the way to go, it needs to be filled.

@alecharp I guess now it is correct

alecharp · 2024-05-13T13:32:51Z

Have you tested you code and its behavior?
Locally, and re-reading the code again, I don't see any proof that it's actually answering #493. There is no field in the /api/scores for each plugin that provide the previous score of the plugins.

AayushSaini101 · 2024-05-15T11:38:31Z

Have you tested you code and its behavior? Locally, and re-reading the code again, I don't see any proof that it's actually answering #493. There is no field in the /api/scores for each plugin that provide the previous score of the plugins.

@alecharp Yes i have tested this, and also pasted the screenshoot in the comment #495 (comment) weird, migth be due to new changes in master branch causing no output. I will check

Added extra field to tell the change in the score

e55610c

AayushSaini101 marked this pull request as draft March 22, 2024 08:00

AayushSaini101 and others added 2 commits March 25, 2024 13:12

Remove checkstyle violation

ecf4283

Merge branch 'main' into issue-493

aedc140

AayushSaini101 added 2 commits March 25, 2024 13:40

Update Score.java

6f1b88b

Merge branch 'main' into issue-493

bd971c6

AayushSaini101 added 2 commits March 25, 2024 14:15

Add extra field

b991cd1

Update score

be0e73d

AayushSaini101 added 2 commits March 28, 2024 14:26

change

6a9b550

change

04efbb3

AayushSaini101 force-pushed the issue-493 branch from 49a1411 to 04efbb3 Compare March 28, 2024 08:57

Restore old application.yml

bec9059

alecharp requested changes Mar 28, 2024

View reviewed changes

AayushSaini101 added 3 commits March 28, 2024 20:58

format all

2c609b9

Fix test cases

ebc1e2e

revert old values

719d899

AayushSaini101 added 3 commits March 30, 2024 15:26

Restore application.yml

47688ed

restore details file

c4fe0bd

Restore jenkins file

9057f43

Merge branch 'main' into issue-493

bfa25f5

AayushSaini101 marked this pull request as ready for review March 30, 2024 10:05

renaming methods

30074e1

alecharp reviewed Apr 2, 2024

View reviewed changes

alecharp marked this pull request as draft April 2, 2024 13:57

Add test cases

78c87f4

AayushSaini101 marked this pull request as ready for review April 2, 2024 15:57

alecharp requested changes Apr 3, 2024

View reviewed changes

AayushSaini101 and others added 3 commits April 3, 2024 22:21

improve test cases

1ca2960

Reformat the file

67d5b2d

Merge branch 'main' into issue-493

22bb0cb

alecharp added the enhancement New feature or request label Apr 23, 2024

alecharp marked this pull request as draft April 23, 2024 15:35

AayushSaini101 marked this pull request as ready for review May 8, 2024 04:28

Providing score trend on plugins enhancement #495

Are you sure you want to change the base?

Providing score trend on plugins enhancement #495

Conversation

AayushSaini101 commented Mar 22, 2024 • edited by alecharp

Description

Testing Done

Submitter checklist

jmMeessen commented Mar 22, 2024 • edited

alecharp commented Mar 22, 2024

AayushSaini101 commented Mar 25, 2024

alecharp commented Mar 25, 2024

AayushSaini101 commented Mar 25, 2024 • edited

AayushSaini101 commented Mar 25, 2024

alecharp commented Mar 25, 2024

AayushSaini101 commented Mar 25, 2024

alecharp commented Mar 25, 2024

AayushSaini101 commented Mar 25, 2024

alecharp commented Mar 25, 2024

AayushSaini101 commented Mar 25, 2024

AayushSaini101 commented Mar 28, 2024 • edited

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AayushSaini101 commented Mar 28, 2024 • edited

AayushSaini101 commented Mar 30, 2024 • edited

alecharp left a comment

Choose a reason for hiding this comment

AayushSaini101 commented Apr 2, 2024 • edited

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AayushSaini101 commented Apr 24, 2024

alecharp commented Apr 24, 2024

AayushSaini101 commented May 3, 2024

alecharp commented May 13, 2024

AayushSaini101 commented May 13, 2024

alecharp commented May 13, 2024

alecharp commented May 13, 2024

AayushSaini101 commented May 13, 2024

alecharp commented May 13, 2024

AayushSaini101 commented May 15, 2024

AayushSaini101 commented Mar 22, 2024 •

edited by alecharp

jmMeessen commented Mar 22, 2024 •

edited

AayushSaini101 commented Mar 25, 2024 •

edited

AayushSaini101 commented Mar 28, 2024 •

edited

AayushSaini101 commented Mar 28, 2024 •

edited

AayushSaini101 commented Mar 30, 2024 •

edited

AayushSaini101 commented Apr 2, 2024 •

edited