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: separate products_obsolete MongoDB collection for obsolete products #8277

Merged
merged 9 commits into from
Apr 5, 2023

Conversation

stephanegigandet
Copy link
Contributor

Fixes #8078

This moves obsolete products to a new products_obsolete collection.

?obsolete=1 can be added to URLs to get them to return results from the products_obsolete collection.

@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.

@stephanegigandet
Copy link
Contributor Author

There are a few things that I didn't do yet. e.g. making some scripts or actions run on both products and products_obsolete collections (for instance when we rename the userid of users who want to have their username deleted).

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #8277 (0ee9ca3) into main (bfc1fe2) will decrease coverage by 0.01%.
The diff coverage is 25.71%.

@@            Coverage Diff             @@
##             main    #8277      +/-   ##
==========================================
- Coverage   46.61%   46.60%   -0.01%     
==========================================
  Files         106      106              
  Lines       20787    20797      +10     
  Branches     4696     4700       +4     
==========================================
+ Hits         9689     9693       +4     
- Misses       9946     9949       +3     
- Partials     1152     1155       +3     
Impacted Files Coverage Δ
lib/ProductOpener/Display.pm 4.67% <0.00%> (ø)
lib/ProductOpener/Food.pm 61.30% <ø> (ø)
lib/ProductOpener/Products.pm 42.29% <35.71%> (-0.08%) ⬇️
lib/ProductOpener/Data.pm 56.14% <57.14%> (+0.58%) ⬆️

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

@stephanegigandet
Copy link
Contributor Author

Added a change in the CSV export to include obsolete products.

@alexgarel
Copy link
Member

There are a few things that I didn't do yet. e.g. making some scripts or actions run on both products and products_obsolete collections (for instance when we rename the userid of users who want to have their username deleted).

Added a reference on #8270

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

I added some concerns about robustness, I let you decide.

Comment on lines +80 to +81
$products_collection->delete_one({"_id" => $product_ref->{_id}});
$obsolete_products_collection->replace_one({"_id" => $product_ref->{_id}}, $product_ref, {upsert => 1});
Copy link
Member

Choose a reason for hiding this comment

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

Ain't it more prudent, especially for a migration, to do these operations on the inverse: first insert then remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll invert the operations.

previous_products_collection => $previous_products_collection
}
) if $log->is_debug();
$previous_products_collection->delete_one({"_id" => $product_ref->{_id}});
Copy link
Member

Choose a reason for hiding this comment

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

I'm asking myself if it wouldn't be better to do this at the end (maybe just store that we want to remove it here, but do it after insertion in its new collection).

Copy link
Member

Choose a reason for hiding this comment

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

Also shan't we also consider the case of marked obsolete and moved to another collection (at least avoid a too complex situation, by eg. ignoring the obsolete status change in this case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move the delete after the insert.

The case of changed obsolete status and moved to another collection should work as-is (the product will be deleted twice from the previous collection, but that's not a problem).

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

sonarqubecloud bot commented Apr 5, 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.

Hide by default the obsolete products withdrawn from the market
4 participants