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

feat: Revert product to a previous revision (API + upcoming website integration for moderators) #9800

Merged
merged 24 commits into from
Mar 8, 2024

Conversation

stephanegigandet
Copy link
Contributor

@stephanegigandet stephanegigandet commented Feb 14, 2024

Will fix #1405

  • adds a new /api/v3/product_revert API
  • API tests
  • new centralized place (Permissions.pm) to check if a user has permission to do something, instead of having checks for admin or moderator status everywhere in the code
  • integration in website (in product history on product page + product edit form), includes a confirmation

image

Still missing:

  • API doc

@stephanegigandet stephanegigandet requested a review from a team as a code owner February 14, 2024 17:48
@stephanegigandet stephanegigandet marked this pull request as draft February 14, 2024 17:48
@github-actions github-actions bot added API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) 🧪 tests 🪶 Apache We use Apache as a server to run Open Food Facts Display Tags labels Feb 14, 2024
@teolemon
Copy link
Member

Can we have some kind of log/notification when a moderator reverts a product ?

@github-actions github-actions bot added 🧬 Taxonomies https://wiki.openfoodfacts.org/Global_taxonomies Product Page JavaScript CSS Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. multilingual products 🌐 Translations labels Feb 16, 2024
@stephanegigandet
Copy link
Contributor Author

Can we have some kind of log/notification when a moderator reverts a product ?

I'm sending a mail to admins, we can revisit once we have a better system in place for events / notifications / patrol.

@stephanegigandet stephanegigandet marked this pull request as ready for review February 16, 2024 17:30
@stephanegigandet stephanegigandet changed the title feat: Revert product to a previous revision (API + upcoming website integration for moderators) -- draft feat: Revert product to a previous revision (API + upcoming website integration for moderators) Feb 16, 2024
@github-actions github-actions bot added 📚 Documentation Documentation issues improve the project for everyone. API v3 labels Feb 19, 2024
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.

Thanks for this great PR !

Overall it's good, but it needs some minor fixes IMO.

html/js/product-history.js Outdated Show resolved Hide resolved
lib/ProductOpener/Permissions.pm Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Good idea to add this module ! This will bring clarity.

lib/ProductOpener/APIProductRevert.pm Outdated Show resolved Hide resolved
Comment on lines 864 to 876
if ($User{moderator}) {
$request_ref->{moderator} = 1;
}
else {
$request_ref->{moderator} = 0;
}

if ($User{pro_moderator}) {
$request_ref->{pro_moderator} = 1;
}
else {
$request_ref->{pro_moderator} = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($User{moderator}) {
$request_ref->{moderator} = 1;
}
else {
$request_ref->{moderator} = 0;
}
if ($User{pro_moderator}) {
$request_ref->{pro_moderator} = 1;
}
else {
$request_ref->{pro_moderator} = 0;
}
$request_ref->{moderator} = !!$User{moderator};
$User{pro_moderator} = !!$request_ref->{pro_moderator};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I wanted to set the value to 0 specifically, but we can use undef instead (!! 0 or !! undef seems to be equal to undef)

Comment on lines 10596 to 10605
$scripts .= <<SCRIPTS
<script>var revert_confirm_message = "$revert_confirm";</script>
<script src="$static_subdomain/js/dist/product-history.js"></script>
SCRIPTS
;

$initjs .= <<JS
activate_product_revert_buttons_in_history();
JS
;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to avoid this kind of JS inclusion (I find it quite annoying) ?

Why isn't it included in the template. Or we could maybe just use a specific class to trigger the JS activity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'm moving it.

@@ -138,7 +138,7 @@ qu:Wira in low quantity
rm:Fat in low quantity
rn:Fat in low quantity
ro:Grăsimi în cantitate mică
ru:Жиры в небольшое количество
ru:Жиры в в малом количестве
Copy link
Member

Choose a reason for hiding this comment

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

? why is the в repeated ?

(see also below)

Copy link
Member

Choose a reason for hiding this comment

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

FYI, it seems to come from taxonomy generations…

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 made a PR to remove this file from GitHub: #9851

tests/integration/api_v3_product_revert.t Outdated Show resolved Hide resolved
Comment on lines 79 to 88
{
test_case => 'revert-product-no-code',
method => 'POST',
path => '/api/v3/product_revert',
body => '{
"rev": 1,
"fields": "code,rev,product_name_en,brands_tags,categories_tags"
}',
ua => $moderator_ua,
},
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a specific return code. Does this mean we send back a 200 ? I think this is important to return a non 20x code (to keep REST like and ease handling by a lot of languages, JS included).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'm changing to 40x codes for errors

@github-actions github-actions bot added API WRITE WRITE API to allow sending product info and image API READ All READ APIs include Product, Search… labels Feb 29, 2024
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.

You did an incredible job ! Thank you.


foreach my $test_ref (@$tests_ref) {

my $response = execute_request($test_ref, $ua);
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 an incredible job at refactoring. Sorry if it feels picky, but I still have a request: I would check the expected code response even in case of setup, it can help understanding that a test fails because a setup test did go wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. So for setup test cases, I now still call check_requests_response (so that we can check expected status code), but I don't save or compare the actual response body.

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Mar 6, 2024
@github-actions github-actions bot removed 🧬 Taxonomies https://wiki.openfoodfacts.org/Global_taxonomies 💥 Merge Conflicts 💥 Merge Conflicts labels Mar 6, 2024
Copy link

sonarcloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@stephanegigandet stephanegigandet merged commit 985e353 into main Mar 8, 2024
14 checks passed
@stephanegigandet stephanegigandet deleted the revert-product branch March 8, 2024 14:55
@alexgarel
Copy link
Member

🎉

john-gom pushed a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪶 Apache We use Apache as a server to run Open Food Facts API READ All READ APIs include Product, Search… API v3 API WRITE WRITE API to allow sending product info and image API Issues related to the Open Food Facts API. More specific labels exist & should be used (API WRITE…) CSS Display 📚 Documentation Documentation issues improve the project for everyone. ✏️ Editing JavaScript multilingual products Product Page Products Tags Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. 🧪 tests 🌐 Translations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add button to revert whole product to previous revision
3 participants