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

Proposed refactor: convert notification bodies to ERB templates #197

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

jvendetti
Copy link
Member

I recently had to figure out why the OBO Foundry synchronization script was failing, along with the synchronization report that's supposed to go out via email at script completion. While looking at code in the Notifications class, I was bothered by the proliferation of heredocs and gsubbing. I propose using ERB templates to represent the notification bodies, and this pull request implements one example:

  • Adds an ERB template to a top-level views/emails directory for the body of the OBO Foundry synchronization report
  • Modifies the obofoundry_sync method to set the necessary variables and instantiate / complete the template

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 80.13%. Comparing base (604e5cb) to head (bd2ad7f).

Files Patch % Lines
lib/ontologies_linked_data/utils/notifications.rb 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #197      +/-   ##
===========================================
+ Coverage    80.07%   80.13%   +0.06%     
===========================================
  Files           65       65              
  Lines         5004     4999       -5     
===========================================
- Hits          4007     4006       -1     
+ Misses         997      993       -4     
Flag Coverage Δ
unittests 80.13% <0.00%> (+0.06%) ⬆️

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.

@alexskr
Copy link
Member

alexskr commented Apr 24, 2024

great idea! This will definitely help tidy up the code.

I've noticed that some non-Rails projects store .erb files in the templates directory, though I'm uncertain if it offers any clarity over using the views directory.

@jvendetti
Copy link
Member Author

Templates would also be a good name for the directory

@alexskr alexskr merged commit 5b44098 into develop Oct 1, 2024
6 of 8 checks passed
@jvendetti
Copy link
Member Author

@alexskr - please don't merge my pull requests without asking me first. As the title points out, this is a proposed refactor. If others approved, the intention was to add a series of follow-up commits that converted the rest of the notification bodies to ERB templates. All of this work should have been done as part of a single pull request.

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.

2 participants