Skip to content

Adding NBP push connector#1452

Merged
MrSerth merged 2 commits into
openHPI:masterfrom
Mathis-Z:nbp-push-connector
Jul 30, 2024
Merged

Adding NBP push connector#1452
MrSerth merged 2 commits into
openHPI:masterfrom
Mathis-Z:nbp-push-connector

Conversation

@Mathis-Z
Copy link
Copy Markdown
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
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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 👍.

Comment thread app/jobs/nbp_push_all_job.rb Outdated
Comment thread app/models/task.rb Outdated
Comment thread app/models/task.rb Outdated
Comment thread app/models/task.rb Outdated
Comment thread config/environments/production.rb Outdated
Comment thread db/schema.rb
Comment thread app/jobs/application_job.rb
Comment thread app/jobs/application_job.rb
Comment thread app/jobs/application_job.rb
Comment thread app/jobs/nbp_push_job.rb Outdated
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread config/puma.rb Outdated
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread app/models/task.rb Outdated
Comment thread db/migrate/20240609104039_create_solid_queue_tables.solid_queue.rb
@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.

Comment thread lib/nbp/push_connector.rb Outdated
Comment thread app/jobs/application_job.rb
@Mathis-Z

This comment was marked as resolved.

@MrSerth

This comment was marked as resolved.

Copy link
Copy Markdown
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 🙂

Comment thread app/jobs/nbp_sync_all_job.rb
Comment thread app/services/lom_service/export_lom.rb
Comment thread app/services/lom_service/nbp_scrubber.rb
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread lib/nbp/push_connector.rb
Comment thread app/jobs/nbp_sync_all_job.rb Outdated
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread lib/nbp/push_connector.rb
Comment thread lib/nbp/push_connector.rb
Comment thread app/jobs/nbp_sync_all_job.rb
Comment thread docs/LOCAL_SETUP.md Outdated
Comment thread lib/nbp/push_connector.rb Outdated
Comment thread spec/fixtures/files/nbp/nodes.json Outdated
@MrSerth
Copy link
Copy Markdown
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.

Comment thread lib/nbp/push_connector.rb Outdated
@Mathis-Z
Copy link
Copy Markdown
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
Copy Markdown
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
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