1254 remove attempts fks #1453
1254 remove attempts fks #1453
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1453 +/- ##
==========================================
+ Coverage 87.50% 87.58% +0.07%
==========================================
Files 214 214
Lines 6346 6370 +24
==========================================
+ Hits 5553 5579 +26
+ Misses 793 791 -2 ☔ View full report in Codecov by Sentry. |
* Added additional functionality to replace lost delete cascade behaviour.
rorymckinley
force-pushed
the
1254-remove-attempts-fks
branch
from
41a9d1a
to
aa365af
Compare
rorymckinley
force-pushed
the
1254-remove-attempts-fks
branch
from
324c498
to
c731580
Compare
rorymckinley
force-pushed
the
1254-remove-attempts-fks
branch
from
c731580
to
8c6dbee
Compare
|
Please note: Should this PR be reactivated, there are some code changes that need to be made. The biggest one is to remove all the plumbing added for Mox and use https://hex.pm/packages/mock instead. There are also some changes required when pattern matching against response from |
|
|
||
| case result do | ||
| {:ok, %{user: deleted_user}} -> {:ok, deleted_user} | ||
| {:error, _, changeset} -> {:error, changeset} |
There was a problem hiding this comment.
Match against all the elements of the Ecto.Multi error response tuple:
{:error, _failed_operation, changeset, _changes_so_far} -> {:error, changeset}
|
|
||
| case result do | ||
| {:ok, %{attempt: modified_attempt}} -> {:ok, modified_attempt} | ||
| {:error, _, changeset} -> {:error, changeset} |
There was a problem hiding this comment.
Match against all the elements of the Ecto.Multi error response tuple:
{:error, _failed_operation, changeset, _changes_so_far} -> {:error, changeset}
|
|
||
| case result do | ||
| {:ok, %{attempts: deleted_attempts}} -> {:ok, deleted_attempts} | ||
| {:error, _, changeset} -> {:error, changeset} |
There was a problem hiding this comment.
Match against all the elements of the Ecto.Multi error response tuple:
{:error, _failed_operation, changeset, _changes_so_far} -> {:error, changeset}
|
@rorymckinley , should this still be in "draft mode" or is it ready for review and merge? |
Notes for the reviewer
Removes the FK constraint between
attempt_runs&attemptsas well as the FK betweenlog_linesandattempts. I added functionality toAttemptsto replicate theON DELETE CASCADEbehaviour lost as a result of removing the FK.I also simplified the tests in
AccountsTestandQueueTestas much of what they were testing is now encapsulated in he methods added toAttemptsand so the detailed tests now live inAttemptsTest(sorry, makes for a bit of a messy diff - happy to talk through the changes).Note: I could not test the rollback behaviour for
Attempts.delete_for_useras I could not find a way to trigger a rollback from a test. Open to any suggestions as to how I can improve the situation.I added mocking so that I could test the response when there is an error.
Related issue
Fixes #
Review checklist