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

fix: refactor protection of data imported from producers #8274

Merged
merged 15 commits into from
Apr 4, 2023

Conversation

stephanegigandet
Copy link
Contributor

This PR solves some issues from #6816

  • Protection on the web now works (bug fixed)
  • Protection in the API is more precise (only fields sent by the producer are protected)
  • Started to refactor so that the protection works the same way in the API and on the web
  • Added tests to verify that the protection works as intended

@stephanegigandet stephanegigandet requested a review from a team as a code owner March 30, 2023 16:26
@stephanegigandet stephanegigandet changed the title fix: refactor protection of data imported from producers #6816 fix: refactor protection of data imported from producers Mar 30, 2023
@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #8274 (e4d58c7) into main (bfc1fe2) will decrease coverage by 0.01%.
The diff coverage is 38.46%.

@@            Coverage Diff             @@
##             main    #8274      +/-   ##
==========================================
- Coverage   46.61%   46.60%   -0.01%     
==========================================
  Files         106      106              
  Lines       20787    20797      +10     
  Branches     4696     4699       +3     
==========================================
+ Hits         9689     9692       +3     
- Misses       9946     9953       +7     
  Partials     1152     1152              
Impacted Files Coverage Δ
lib/ProductOpener/TestDefaults.pm 100.00% <ø> (ø)
lib/ProductOpener/Food.pm 61.15% <25.00%> (-0.15%) ⬇️
lib/ProductOpener/APIProductWrite.pm 17.77% <28.57%> (+0.91%) ⬆️
lib/ProductOpener/Products.pm 42.37% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Really cool implementation.

I have some comments. The most important being about the 1 year rule.

Also it would be great if we could act on the web form (and the mobile form) to avoid users to try to modify those fields if they can't ! I see a lot of complaints coming.

lib/ProductOpener/APIProductWrite.pm Outdated Show resolved Hide resolved
lib/ProductOpener/APIProductWrite.pm Show resolved Hide resolved
tests/integration/protected_product.t Show resolved Hide resolved
tests/integration/protected_product.t Outdated Show resolved Hide resolved
method => 'GET',
path => '/api/v2/product/0200000000135',
},
];
Copy link
Member

Choose a reason for hiding this comment

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

You did not test,if it works for moderators on protected products.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexgarel I added some tests for moderators.

stephanegigandet and others added 5 commits April 3, 2023 15:07
Co-authored-by: Alex Garel <alex@garel.org>
Co-authored-by: Alex Garel <alex@garel.org>
Co-authored-by: Alex Garel <alex@garel.org>
@stephanegigandet
Copy link
Contributor Author

Also it would be great if we could act on the web form (and the mobile form) to avoid users to try to modify those fields if they can't ! I see a lot of complaints coming.

Related:
#6721
#2012

And I filed #8282

@@ -2145,24 +2149,24 @@
"ingredients" : [
{
"id" : "es:apple",
"percent_estimate" : 66.6666666666667,
"percent_estimate" : "66.6666666666667",
Copy link
Member

Choose a reason for hiding this comment

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

Strange that strings strikes back ☹️

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Cool.

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Apr 4, 2023
@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 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

No Coverage information No Coverage information
No Duplication information No Duplication information

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

Successfully merging this pull request may close these issues.

None yet

3 participants