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

Move logic from entity base to MongoRepo #466

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 26, 2024

Continuation of #459


This change is Reviewable

@Enkidu93 Enkidu93 requested a review from ddaspit August 26, 2024 16:19
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 35 lines in your changes missing coverage. Please review.

Project coverage is 56.64%. Comparing base (cfb7dda) to head (5e5364a).
Report is 2 commits behind head on main.

Files Patch % Lines
...c/DataAccess/src/SIL.DataAccess/MongoRepository.cs 55.69% 33 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
- Coverage   56.69%   56.64%   -0.05%     
==========================================
  Files         275      275              
  Lines       14120    14146      +26     
  Branches     1895     1895              
==========================================
+ Hits         8005     8013       +8     
- Misses       5531     5548      +17     
- Partials      584      585       +1     

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

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

@johnml1135
Copy link
Collaborator

Can you add tests to verify the "entity not found" behavior for webhooks, translation engines and assessment engines?

@johnml1135
Copy link
Collaborator

So we can make sure all of Mike Pelley's tests pass.

@Enkidu93
Copy link
Collaborator Author

So we can make sure all of Mike Pelley's tests pass.

Yep, good call. I'll go ahead and do that.

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @ddaspit)

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)


src/DataAccess/src/SIL.DataAccess/MongoRepository.cs line 22 at r2 (raw file):

            return await _collection.AsQueryable().FirstOrDefaultAsync(filter, cancellationToken).ConfigureAwait(false);
        }
        catch (FormatException)

I would guess that this same exception can occur on other methods as well, i.e. ExistsAsync, UpdateAsync, DeleteAsync, etc.

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)


src/DataAccess/src/SIL.DataAccess/MongoRepository.cs line 22 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I would guess that this same exception can occur on other methods as well, i.e. ExistsAsync, UpdateAsync, DeleteAsync, etc.

Done. I think I put them wherever appropriate. I'm not sure that all of errors are reachable via the API, but since the DataAccess is modular, I guess it makes sense. Which reminds me: We'll need to release a new version for these to take effect then, right?

@johnml1135
Copy link
Collaborator

src/DataAccess/src/SIL.DataAccess/MongoRepository.cs line 205 at r3 (raw file):

            return default;
        }
    }

If there are 6? try-catches added and only 3 test cases, why?

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


src/DataAccess/src/SIL.DataAccess/MongoRepository.cs line 22 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. I think I put them wherever appropriate. I'm not sure that all of errors are reachable via the API, but since the DataAccess is modular, I guess it makes sense. Which reminds me: We'll need to release a new version for these to take effect then, right?

We actually don't have to release it anymore, since all of the assemblies that use SIL.DataAccess are in the same repo and no longer have to get the package from NuGet. We can still release it just to keep it up-to-date on NuGet.


src/DataAccess/src/SIL.DataAccess/MongoRepository.cs line 205 at r3 (raw file):

Previously, johnml1135 (John Lambert) wrote…

If there are 6? try-catches added and only 3 test cases, why?

The integration tests are expensive. I think the tests that Eli added are probably sufficient.

@johnml1135
Copy link
Collaborator

:lgtm:

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@johnml1135 johnml1135 force-pushed the no_500_on_serialization_failure branch from 5e5364a to 321cec3 Compare August 28, 2024 12:21
@johnml1135 johnml1135 merged commit 8857819 into main Aug 28, 2024
1 of 2 checks passed
@johnml1135 johnml1135 deleted the no_500_on_serialization_failure branch August 28, 2024 12:22
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