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

Adding NBP push connector #1452

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Conversation

Mathis-Z
Copy link
Contributor

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.

@Mathis-Z Mathis-Z requested a review from MrSerth June 10, 2024 20:33
@Mathis-Z Mathis-Z self-assigned this Jun 10, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.77%. Comparing base (4d760dd) to head (f4a38be).
Report is 21 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@MrSerth MrSerth left a 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 👍.

app/jobs/nbp_push_all_job.rb Outdated Show resolved Hide resolved
app/models/task.rb Outdated Show resolved Hide resolved
app/models/task.rb Outdated Show resolved Hide resolved
app/models/task.rb Outdated Show resolved Hide resolved
config/environments/production.rb Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
app/jobs/application_job.rb Show resolved Hide resolved
app/jobs/application_job.rb Show resolved Hide resolved
app/jobs/application_job.rb Show resolved Hide resolved
app/jobs/nbp_push_job.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
config/puma.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
app/models/task.rb Outdated Show resolved Hide resolved
@Mathis-Z Mathis-Z force-pushed the nbp-push-connector branch 2 times, most recently from fe3c1fe to 961f8a3 Compare June 17, 2024 18:08
@Mathis-Z Mathis-Z force-pushed the nbp-push-connector branch 6 times, most recently from a57a80b to 392518a Compare June 26, 2024 08:10
@Mathis-Z

This comment was marked as resolved.

@MrSerth

This comment was marked as resolved.

@Mathis-Z

This comment was marked as resolved.

@MrSerth

This comment was marked as resolved.

lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
@Mathis-Z

This comment was marked as resolved.

@MrSerth

This comment was marked as resolved.

Copy link
Member

@MrSerth MrSerth left a 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 🙂

app/jobs/nbp_sync_all_job.rb Show resolved Hide resolved
app/services/lom_service/export_lom.rb Show resolved Hide resolved
app/services/lom_service/nbp_scrubber.rb Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
@Mathis-Z Mathis-Z force-pushed the nbp-push-connector branch 2 times, most recently from aebd226 to 0c6be5b Compare July 12, 2024 13:19
app/jobs/nbp_sync_all_job.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
docs/LOCAL_SETUP.md Outdated Show resolved Hide resolved
lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
@MrSerth
Copy link
Member

MrSerth commented Jul 24, 2024

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.

lib/nbp/push_connector.rb Outdated Show resolved Hide resolved
@Mathis-Z
Copy link
Contributor Author

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.

Copy link
Member

@MrSerth MrSerth left a 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.

@MrSerth MrSerth merged commit 5a2bd24 into openHPI:master Jul 30, 2024
9 checks passed
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.

3 participants