Don't override Equals, GetHashCode

[imnasnainaec]

We thought they were only used in testing, but they snuck into production via 4 uses of .Remove as well as uses of .Contains in other model methods.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.26%. Comparing base (7d92ffb) to head (e888c9f).
Report is 28 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3660      +/-   ##
==========================================
- Coverage   74.24%   73.26%   -0.99%     
==========================================
  Files         285      285              
  Lines       11143    10629     -514     
  Branches     1373     1322      -51     
==========================================
- Hits         8273     7787     -486     
+ Misses       2476     2452      -24     
+ Partials      394      390       -4     

Flag	Coverage Δ
backend	82.81% <100.00%> (-1.17%)	⬇️
frontend	65.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasonleenaylor]

I had two comments worth considering, but overall

Reviewed 33 of 47 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

---

Backend/Services/WordService.cs line 109 at r2 (raw file):

            var rmCount = wordWithAudioToDelete.Audio.RemoveAll(a => a.FileName == fileName);
            if (rmCount == 0)

There is nothing at all wrong with this change, but I found it less clear than the original. So unless we expect Audio to have duplicate entries with the same filename I would slightly prefer the original. Should be bring @andracc in as a tie breaker? 😄

Code quote:

            var rmCount = wordWithAudioToDelete.Audio.RemoveAll(a => a.FileName == fileName);
            if (rmCount == 0)

---

Backend.Tests/Controllers/UserRoleControllerTests.cs line 241 at r2 (raw file):

        [Test]
        public async Task TestUpdateUserRoleNoChange()

So this test no longer had any value?

[hahn-kev]

this test will pass even if the clone is not a deep clone. So you might want to check that Parent and Child Ids don't refer to the same lists. Just a thought though.

[imnasnainaec]

Ah, yes. This is why you use

            assertionChain
                .ForCondition(!ReferenceEquals(comparands.Subject, comparands.Expectation))
                .BecauseOf(context.Reason)
                .FailWith("Subject and Expectation for {0} should not reference the same instance in memory.", context.CurrentNode.Description);

I think for now I'll leave it and make an issue for considering that with all our Clone tests. The main purpose of the tests is to make sure no property was left out, but it would be valuable to make sure they've all been truly cloned.

[imnasnainaec]

#3669

[hahn-kev]

I'll bet that felt good to delete all that code

[imnasnainaec]

Reviewable status: 44 of 47 files reviewed, 1 unresolved discussion (waiting on @hahn-kev and @jasonleenaylor)

---

Backend.Tests/Controllers/UserRoleControllerTests.cs line 241 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

So this test no longer had any value?

Very little. Most of our Repository Mocks don't have a "NotModified" case, and this test depends on the

            if (foundUserRole.ContentEquals(userRole))
            {
                return Task.FromResult(ResultOfUpdate.NoChange);
            }

case of the Update method in UserRoleRepositoryMock, which was the only reason we had UserRole.ContentEquals defined. So I thought I'd remove it to have UserRoleRepositoryMock more consistent with the other Mocks.

---

Backend/Services/WordService.cs line 109 at r2 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

There is nothing at all wrong with this change, but I found it less clear than the original. So unless we expect Audio to have duplicate entries with the same filename I would slightly prefer the original. Should be bring @andracc in as a tie breaker? 😄

Reverted.

[imnasnainaec]

Ah, yes. This is why you use

            assertionChain
                .ForCondition(!ReferenceEquals(comparands.Subject, comparands.Expectation))
                .BecauseOf(context.Reason)
                .FailWith("Subject and Expectation for {0} should not reference the same instance in memory.", context.CurrentNode.Description);

I think for now I'll leave it and make an issue for considering that with all our Clone tests. The main purpose of the tests is to make sure no property was left out, but it would be valuable to make sure they've all been truly cloned.

[jasonleenaylor]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hahn-kev)

[imnasnainaec]

Dismissed @hahn-kev from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)