🌳 Feature - Enhance plugin to include "deletedId" on deleted docs#2
🌳 Feature - Enhance plugin to include "deletedId" on deleted docs#2taherashorna1 merged 18 commits intomasterfrom
Conversation
AndrewFinlay
left a comment
AndrewFinlay
left a comment
There was a problem hiding this comment.
This is looking really good Tahera. It might look like there's a heap of feedback on this but it's really just a bunch of tweaks here and there. It's a bit of a tricky one this as the library is obviously really old now and that can make it hard to know what we should update and what we should leave be.
That said I think we should be safe to make some of our own updates to it, and eventually we should make it async/await and simply drop the callback based stuff. We'd have to do that anyway if we're ever going to upgrade Mongoose.
AndrewFinlay
left a comment
AndrewFinlay
left a comment
There was a problem hiding this comment.
This is great, thanks for the hard work!
| test: | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-20.04, ubuntu-22.04] | ||
| node: [16, 18, 20] | ||
| mongodb-version: ['4.4.18', '5.0.14', '6.0.4'] | ||
| name: Node ${{ matrix.node }} MongoDB ${{ matrix.mongodb-version }} |
There was a problem hiding this comment.
🤔 I don't know how much of this we can change because if we merge this back to the upstream repo then it will still need to run the same kind of multi-os multi-runtime checks.
That said we could probably update some of the targets in here. I don't think any of the ubuntu-20.04 targets ran so we should be good to drop that. If you look at https://github.com/actions/runner-images it will list what images are available for the os setting. I'd go with Ubuntu 22 & 24.
According to the link above it says that it supports the latest 3 LTS nodejs versions. If you take a look at https://nodejs.org/en/about/previous-releases you'll see a chart showing the currency of Node releases. Only even numbered releases are ever used as Long Term Support (LTS) releases, 26 doesn't exist yet but it's planned, 24 is in the current state until October meaning it won't be an active LTS release until then, so to test with the last three LTS releases right now would be 18, 20, and 22.
Finally it lists a bunch of MongoDB versions. I think to remove any of the old versions without a major bump for the library would be against semver as you're removing a guarantee of compatibility. So your best course of action for now is to add Mongo 7 & 8. I think what's available will depend on what's included in the Ubuntu image, Ubuntu 22 is known as Jammy & Ubuntu 24 is known as noble and I don't know if noble has support going all the way back. That said I think the latest version for Mongo 7 is 7.0.21, and for Mongo 8 it's 8.0.11. If you run into trouble with noble I'd drop Ubuntu 24 from the matrix until you can do a major bump for mongoose delete.
Co-authored-by: AndrewFinlay <AndrewFinlay@users.noreply.github.com>
Co-authored-by: AndrewFinlay <AndrewFinlay@users.noreply.github.com>
package.json
Outdated
| "main": "index.js", | ||
| "scripts": { | ||
| "test": "mocha", | ||
| "test:ci": "MONGODB_TEST_URI=mongodb://localhost:27017/lighthouse_mongoose_delete_test?directConnection=true mocha --exit", |
There was a problem hiding this comment.
❓ Is this needed to run as a ci test?
| // mongodb: { deleted: true, name: 'Bitey', deletedId: ObjectId("61fc42a56b4a6670076b16bf")} | ||
|
|
||
| // Find all pets deleted as part of this action | ||
| const deletedPets = await Pet.findWithDeleted({ deletedId: actionId }); |
There was a problem hiding this comment.
🤏 I think findWithDeleted might be a LH function yeah? That probably means you'll need to have your own equivalent implementation in your docs.
There was a problem hiding this comment.
findWithDeleted is a mongoose-delete function that can be used instead of find, so I think this is fine here?
AndrewFinlay
left a comment
AndrewFinlay
left a comment
There was a problem hiding this comment.
Looks good, nice work on this. I'll check in with Todd next time I speak with him about putting this up as a PR to the original repo
🌳 Feature - Enhance plugin to include "deletedId" on deleted docs
AB#254511
We are upgrading mongoose-delete plugin to add deletedId option for the restoration of locations
This pull request introduces support for a new
deletedIdfield in themongoose_deleteplugin. The changes include updates to the schema, methods, and static functions to handle thedeletedIdfield, as well as comprehensive tests to validate the new functionality.Schema and Method Enhancements:
deletedIdfield in the schema, which can be enabled via thedeletedIdoption. This field is indexed if specified. (index.js: [1] [2]deleteanddeleteByIdmethods to accept and processdeletedIdalongsidedeletedBy. ThedeletedIdvalue is prioritized if both are provided. (index.js: [1] [2] [3]restoremethod to unset thedeletedIdfield upon restoration. (index.js: index.jsL290-R351)$unsetoperation to includedeletedIdwhen restoring documents. (index.js: index.jsL315-R371)Tests:
deletedIdfield, including scenarios for deletion, restoration, and type customization. These tests ensure proper handling ofdeletedIdin various configurations. (test/index.js: test/index.jsR551-R701)