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

Providing score trend on plugins enhancement #495

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

AayushSaini101
Copy link
Contributor

@AayushSaini101 AayushSaini101 commented Mar 22, 2024

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

@AayushSaini101 AayushSaini101 marked this pull request as draft March 22, 2024 08:00
@jmMeessen
Copy link
Contributor

jmMeessen commented Mar 22, 2024

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
Copy link
Collaborator

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

@AayushSaini101
Copy link
Contributor Author

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
Copy link
Collaborator

I fixed the description.

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

@AayushSaini101
Copy link
Contributor Author

AayushSaini101 commented Mar 25, 2024

#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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@AayushSaini101
Copy link
Contributor Author

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,
image

@alecharp
Copy link
Collaborator

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

@AayushSaini101
Copy link
Contributor Author

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.
image 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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

AayushSaini101 commented Mar 28, 2024

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

Copy link
Collaborator

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

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.

.DS_Store Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

Jenkinsfile Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

war/.yarnrc.yml Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

war/yarn.lock Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved @alecharp

@AayushSaini101
Copy link
Contributor Author

AayushSaini101 commented Mar 28, 2024

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
Copy link
Contributor Author

AayushSaini101 commented Mar 30, 2024

image Here is the previous score for every plugin

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

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

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

@alecharp alecharp marked this pull request as draft April 2, 2024 13:57
@AayushSaini101
Copy link
Contributor Author

AayushSaini101 commented Apr 2, 2024

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

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

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 60 to 79
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
));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Comment on lines 141 to 148
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alecharp updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alecharp updated

100, 1, List.of("There is no active security advisory for the plugin."))),
1));

final ZonedDateTime latestTimePlugin2 = ZonedDateTime.now().minusMinutes(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecharp removed

scoreP1Date.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME),
scoreP1Date.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME)),
false));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

reactivate the formatter before closing the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecharp Cannot format manually once I edit the file, spotless causing build failure
Screenshot 2024-04-03 at 10 05 02 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @alecharp

@alecharp alecharp added the enhancement New feature or request label Apr 23, 2024
@alecharp alecharp marked this pull request as draft April 23, 2024 15:35
@AayushSaini101
Copy link
Contributor Author

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

@alecharp
Copy link
Collaborator

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

@AayushSaini101
Copy link
Contributor Author

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

Done @alecharp

@AayushSaini101 AayushSaini101 marked this pull request as ready for review May 8, 2024 04:28
@alecharp
Copy link
Collaborator

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

@AayushSaini101
Copy link
Contributor Author

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

Added more points @alecharp thanks

@alecharp
Copy link
Collaborator

And still, there is a TODO

@alecharp
Copy link
Collaborator

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
Copy link
Contributor Author

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.

image @alecharp I guess now it is correct

@alecharp
Copy link
Collaborator

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
Copy link
Contributor Author

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

@alecharp
Copy link
Collaborator

@AayushSaini101 do you think you will be able to complete this pull request?

@AayushSaini101
Copy link
Contributor Author

@AayushSaini101 do you think you will be able to complete this pull request?

Sorry @alecharp I was on the vacations. I can start working in the next week

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

Successfully merging this pull request may close these issues.

Providing score trend on plugins
3 participants