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

Update uploads/documents deletion to use soft delete #2556

Merged
merged 40 commits into from Sep 4, 2019

Conversation

@ralren
Copy link
Contributor

commented Aug 19, 2019

Description

A follow-up to this PR which makes use of the new soft delete method.

Reviewer Notes

You might notice that I removed the transaction within SoftDestroy(). This is initially due to the conflict of having a transaction nested within another in the SaveMoveDocument() function within move_document.go. While @jim and I entertained other solutions, I decided it was best to remove the transaction call within SoftDestroy() as it should be treated as an extension of pop and its methods do not contain such a call either.

Setup

make server_run
make office_client_run

Mess around and change the statuses of move documents. Feel free to run the new tests.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Chris Coles and others added 26 commits Jul 31, 2019
Chris Coles
Adds `deleted_at` to tables - documents, move_documents, uploads, wei…
…ght_ticket_set_documents, moving_expense_documents.

@ralren ralren requested review from jim, chrisgilmerproj, mkrump and kahlouie Aug 19, 2019

@@ -39,7 +40,7 @@ func (d *Document) Validate(tx *pop.Connection) (*validate.Errors, error) {
func FetchDocument(ctx context.Context, db *pop.Connection, session *auth.Session, id uuid.UUID) (Document, error) {

var document Document

This comment has been minimized.

Copy link
@mkrump

mkrump Aug 21, 2019

Contributor

I also had a question about the filtering out of soft deleted objects on line 43. Right now if someone Eager loads a soft deleted object, or uses pop's native Find pop is unaware of the soft delete, so this extra Where("documents.deleted_at is null") enforces soft delete by convention. I've seen other parts of the codebase using pop's callback functions https://github.com/transcom/mymove/blob/master/pkg/models/upload.go#L46-L58, there is an AfterFind callback. Not sure this would work, but I wonder if you could do the filtering of soft deleted objects there and then the user doesn't have know about / remember to add this extra bit of filtering.

This comment has been minimized.

Copy link
@ralren

ralren Aug 21, 2019

Author Contributor

I think this is a viable path to take. I'm just not sure if BeforeCreate() within the upload model is being invoked and, if it is, where it is being invoked. I don't understand how this function operates.

This comment has been minimized.

Copy link
@mkrump

mkrump Aug 21, 2019

Contributor

Sorry I didn't attach the link https://gobuffalo.io/en/docs/db/callbacks. In the case of BeforeCreate it's called right before the creation of an object in db (if you want to see it step into the debugger and create an upload). Like I said not sure that it would work (in the case of AfterFind), but might be another way to ensure the filtering is enforced.

This comment has been minimized.

Copy link
@ralren

ralren Aug 21, 2019

Author Contributor

Thanks, Matt! I do like this option now that I have a better understanding of it. I'm not sure if I will take action now as I want to resolve the other conversation about whether we should allow a conditional to fetch all records or not first.

This comment has been minimized.

Copy link
@mkrump

mkrump Aug 21, 2019

Contributor

sounds good. happy to help you investigate if you decide to look at this.

@codecov

This comment has been minimized.

Copy link

commented Aug 21, 2019

Codecov Report

Merging #2556 into master will increase coverage by 0.2%.
The diff coverage is 84.2%.

@@           Coverage Diff            @@
##           master   #2556     +/-   ##
========================================
+ Coverage    60.3%   60.4%   +0.2%     
========================================
  Files         279     281      +2     
  Lines       15680   15720     +40     
========================================
+ Hits         9442    9490     +48     
+ Misses       5113    5108      -5     
+ Partials     1125    1122      -3
Impacted Files Coverage Δ
pkg/models/weight_ticket_set_document.go 81% <ø> (ø) ⬆️
pkg/models/moving_expense_document.go 73.2% <ø> (ø) ⬆️
...es/move_documents/generic_move_document_updater.go 0% <0%> (ø) ⬆️
pkg/models/personally_procured_move.go 49.1% <100%> (+11.2%) ⬆️
pkg/models/move_documents_extractor.go 92.4% <100%> (+0.4%) ⬆️
pkg/models/document.go 80% <100%> (+53.4%) ⬆️
pkg/models/upload.go 84.4% <100%> (+37.8%) ⬆️
pkg/models/move_document.go 43% <50%> (ø) ⬆️
...services/move_documents/storage_expense_updater.go 69.3% <66.7%> (ø) ⬆️
pkg/db/utilities/utilities.go 90.4% <90.4%> (ø)
... and 11 more
@codecov

This comment has been minimized.

Copy link

commented Aug 21, 2019

Codecov Report

Merging #2556 into master will increase coverage by 0.5%.
The diff coverage is 82.5%.

@@           Coverage Diff            @@
##           master   #2556     +/-   ##
========================================
+ Coverage    56.3%   56.7%   +0.5%     
========================================
  Files         243     244      +1     
  Lines       11721   11792     +71     
========================================
+ Hits         6589    6683     +94     
+ Misses       4457    4429     -28     
- Partials      675     680      +5
Impacted Files Coverage Δ
pkg/models/moving_expense_document.go 58.6% <ø> (ø) ⬆️
...es/move_documents/generic_move_document_updater.go 0% <0%> (ø) ⬆️
pkg/models/shipment_summary_worksheet.go 85.8% <0%> (ø) ⬆️
...g/handlers/internalapi/personally_procured_move.go 81.5% <100%> (ø) ⬆️
pkg/handlers/internalapi/documents.go 59.2% <100%> (ø) ⬆️
pkg/models/weight_ticket_set_document.go 81% <100%> (ø) ⬆️
pkg/models/upload.go 84.4% <100%> (+37.8%) ⬆️
pkg/handlers/internalapi/move_documents.go 70.1% <100%> (ø) ⬆️
pkg/handlers/internalapi/uploads.go 68.1% <100%> (ø) ⬆️
pkg/models/personally_procured_move.go 42.4% <100%> (+11.3%) ⬆️
... and 14 more

@ralren ralren requested review from lynzt, kimallen and chrisrcoles Aug 21, 2019

@ralren

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@kimallen @lynzt @chrisgilmerproj I added you all as reviewers as this will affect all the teams.

@ralren ralren removed the do not merge label Aug 22, 2019

@ralren ralren changed the title [Do not merge] Update uploads/documents deletion to use soft delete Update uploads/documents deletion to use soft delete Aug 22, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

Looking good but needs to be able to query soft-deleted items.

@ralren ralren requested review from chrisgilmerproj, mkrump and kahlouie Aug 22, 2019

@lynzt

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Depending on how we want to query the deleted recs, we might want to add a db index on those columns...

@jim
jim approved these changes Aug 29, 2019
Copy link
Contributor

left a comment

Lookin' good.

Did we end up making a ticket to look at the slow tests? @lynzt might know about any upcoming efforts in that area.

@chrisgilmerproj
Copy link
Contributor

left a comment

I think this looks good for the parts I understand. I would still like to see another dev approve this. Also, consider deploying this to experimental later today and test it to see if it does what you expect on that environment.

@chrisrcoles
Copy link
Contributor

left a comment

LGTM 🚢

@ralren ralren merged commit 337da73 into master Sep 4, 2019

16 checks passed

ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch/go 82.5% of diff hit (target 56%)
Details
codecov/project/go 56.4% (+0.5%) compared to 1fe6a51
Details

@ralren ralren deleted the ren-167443089-update-deletion-to-soft-delete branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.