-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adding NBP push connector #1452
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1452 +/- ##
==========================================
+ Coverage 93.44% 93.77% +0.32%
==========================================
Files 115 122 +7
Lines 2808 2955 +147
==========================================
+ Hits 2624 2771 +147
Misses 184 184 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, this looks really great and robust already! 💪 I've tested it locally and also took a moment to understand the domain. Many of the design decisions you took are well thought and I am fine with them 👍.
fe3c1fe
to
961f8a3
Compare
a57a80b
to
392518a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
392518a
to
09ced8b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
1fe164c
to
cf93790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks! Just some minor code-level comments 🙂
aebd226
to
0c6be5b
Compare
I just pushed some (final) adjustments in a dedicated PR. Mainly, I am further improving the test coverage, raising it to 100%. During the creation of the additional tests, I also noticed a minor bug in the existing specs (regarding the token renewal). Hence, I feel it is worth to cover as many lines as possible. Please have another look; I am happy to finally proceed with merging following your check. |
Thank you for writing the additional tests. I think the PR is ready to be merged. I would either squash all commits or maybe squash all except for "Sanitizing exported html in LOM object description". This would require reordering the commits though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for double-checking. The two commits you suggested sound great. I cannot rebase and reorder them on my mobile right now. Either you do so (allowing me to merge on the go) or I’ll do so later.
81ed67f
to
f4a38be
Compare
This is still missing tests but I thought you could already have a look. It is also missing a config file for solid queue but the defaults are pretty reasonable so maybe we don't even need one.