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

[JAVA] CRJVM207 - 10 MILLIARDS - Customer data must have end-of-life information #170

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

[JAVA] CRJVM207 - 10 MILLIARDS - Customer data must have end-of-life information #170

wants to merge 22 commits into from

Conversation

saidmohamedali
Copy link

No description provided.

dedece35
dedece35 requested changes Apr 11, 2023
View reviewed changes
java-plugin/src/main/java/fr/greencodeinitiative/java/checks/CookieWithoutExpirationRule.java Show resolved Hide resolved
java-plugin/src/main/java/fr/greencodeinitiative/java/checks/CookieWithoutExpirationRule.java Outdated
if (visitorInFile.hasANewCookieWithoutMaxDate())
{
//if we found a cookie that maxDate is not initialized, we report issue
reportIssue(tree, "Avoid not setting MaxAge");
Copy link
Member

Choose a reason for hiding this comment

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

message with a double negative is difficult to understand : please use a simple message like Giving a MaxAge is recommended

java-plugin/src/main/java/fr/greencodeinitiative/java/checks/CookieWithoutExpirationRule.java Outdated
for (String variableName : newCookieVariableName )
{
if (!hasSetMaxAgeForCookiesVariableName.contains(variableName))
//si on n'a pas fait setMaxAge pour ces variables
Copy link
Member

Choose a reason for hiding this comment

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

please write code comments in english. check others please. thx

java-plugin/src/main/java/fr/greencodeinitiative/java/checks/CookieWithoutExpirationRule.java Show resolved Hide resolved
java-plugin/src/test/files/CookieWithoutExpirationCheck.java Outdated
import java.util.Collection;
import java.util.Collections;

class TestClass {// Noncompliant {{Avoid not setting MaxAge}}
Copy link
Member

Choose a reason for hiding this comment

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

why message presnet on class and not on each line of code with a non compliance ? I think this is that we should have.

java-plugin/src/test/java/fr/greencodeinitiative/java/checks/CookieWithExpirationCheckTest.java Show resolved Hide resolved
@dedece35
Copy link
Member

dedece35 commented Apr 11, 2023 •
edited
Loading

Hi @saidmohamedali,

thank you for this PR ... but I saw that you created and closed some 3 old PRs with same modifications (#150 / #168 / #169) related to issue #114. Why do you need to recreate PR ? You can modify your code in the same PR.

@dedece35 dedece35 added 🗃️ rule rule improvment or rule development or bug java 🏆 challenge2023 🏆 Work done during the ecoCode Challenge 2023 👀 👀 review done 👀 👀 review done - waiting for changes ⚖️⚖️ todiscuss_coreteam ⚖️⚖️ labels Apr 11, 2023
@sonarcloud
Copy link

sonarcloud bot commented Apr 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

81.8% 81.8% Coverage
0.0% 0.0% Duplication

@github-actions
Copy link

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jun 24, 2023
@github-actions github-actions bot removed the stale label Sep 16, 2023
@dedece35
Copy link
Member

Hi @saidmohamedali,
do you check DoD list ? please check here https://github.com/green-code-initiative/ecoCode-common/blob/main/doc/starter-pack.md#definition-of-done-of-a-pr

@github-actions
Copy link

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Oct 23, 2023
@github-actions github-actions bot removed the stale label Dec 2, 2023
@dedece35
Copy link
Member

Hi @jhertout, @glalloue, @utarwyn
there is no come back by @saidmohamedali, thus I will rework this PR to push the work.
But, if I analyse the need in our Project Kanban, I saw that implementation isn't complete : see https://github.com/orgs/green-code-initiative/projects/1/views/1?filterQuery=CRJVM207&pane=issue&itemId=16188311
This implementation is quite good for Cookie control, but there is a lack about Session, LocalStorage and Cache.
What do you think ? Can we save this work about Cookie into a new rule anyway ?

@dedece35
Copy link
Member

dedece35 commented Dec 22, 2023 •
edited
Loading

core team decision : we split the need => this PR is only for Cookie and rework implementation because of bad implementation (visitor inspecting too much code instead of construct, method invocation)

@github-actions GitHub Actions
Copy link

This PR has been automatically marked as stale because it has no activity for 60 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions bot added the stale label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utarwyn utarwyn utarwyn left review comments

@dedece35 dedece35 dedece35 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@saidmohamedali saidmohamedali

Labels
🗃️ rule rule improvment or rule development or bug 🏆 challenge2023 🏆 Work done during the ecoCode Challenge 2023 java stale 👀 👀 waiting commiter 👀 👀
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saidmohamedali @dedece35 @utarwyn @btisPro @j12t