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

Ensure duplicate versions are deleted during version sync process #225

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

TimCsaky
Copy link
Contributor

@TimCsaky TimCsaky commented Nov 1, 2023

Description

This PR has some changes to the version syncing, specifically deleting versions.
It addresses an edge case where the coms database contains multiple versions with the same S3 VersionId.
There is also a change to update 'is latest' in the version service delete() method .

we could really do with some regression testing on this before merging.
steps for testing:

  • upload 3 versions of an object to a bucket outside of COMS (eg: using S3 Browser, Cyberduck or Minio)
  • sync object with COMS
  • delete a version, add extra versions, do a soft delete outside of COMS
  • sync object again
  • repeat this test on an object in both a versioned and un-versioned bucket (ideally using bc gov bucket)

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@TimCsaky TimCsaky force-pushed the sync branch 2 times, most recently from a1cd440 to 882fb27 Compare November 1, 2023 21:17
Copy link

github-actions bot commented Nov 1, 2023

Coverage Report

Totals Coverage
Statements: 58.18% ( 2665 / 4581 )
Methods: 48.4% ( 303 / 626 )
Lines: 64.79% ( 1601 / 2471 )
Branches: 51.28% ( 761 / 1484 )

Copy link

codeclimate bot commented Nov 1, 2023

Code Climate has analyzed commit d3b92d2 and detected 119 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 74
Style 18
Bug Risk 27

The test coverage on the diff in this pull request is 76.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 64.7% (0.0% change).

View more on Code Climate.

@TimCsaky TimCsaky marked this pull request as ready for review November 1, 2023 21:31
@TimCsaky TimCsaky marked this pull request as draft November 2, 2023 00:19
@TimCsaky TimCsaky marked this pull request as ready for review November 2, 2023 22:41
@TimCsaky TimCsaky changed the title re-factor version delete process Ensure duplicate versions are deleted during version sync process Nov 2, 2023
app/src/services/version.js Outdated Show resolved Hide resolved
app/src/services/version.js Outdated Show resolved Hide resolved
@kyle1morel kyle1morel merged commit 8e4f854 into master Nov 6, 2023
12 checks passed
@kyle1morel kyle1morel deleted the sync branch November 6, 2023 21:36
@jujaga jujaga mentioned this pull request Nov 21, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants