Move sitewide settings to environment variables (PP-494) #1648

jonathangreen · 2024-02-01T17:19:38Z

Description

Move our sitewide settings to environment variables, and remove the sitewide settings controller. Since this is the last bit of the configuration settings to get switched over, remove the old settings controller as well.

🚨 Before this one can be merged we need:

~~Updates to add new env vars to playbook~~
- PR here: https://github.com/ThePalaceProject/hosting-playbook/pull/568
~~Updated admin UI that removes the sitewide settings screens~~
- PR for this here: Remove sitewide settings configuration (PP-494) circulation-admin#101

Changes to sitewide config:

LOG_LEVEL, LOG_APP_NAME and DATABASE_LOG_LEVEL removed because they were no longer used.
EXCLUDED_AUDIO_DATA_SOURCES removed, as its not used on any of our sites, and it was added long ago when testing overdrive content.
MEASUREMENT_REAPER removed, since we don't have any need to disable this reaper.
PUSH_NOTIFICATIONS_STATUS removed, since its enabled everywhere now anyway.
~~BEARER_TOKEN_SIGNING_SECRET is merged with SECRET_KEY, since they have a similar function and it doesn't seem like we need both of them.~~
STATIC_FILE_CACHE_TIME removed, since we don't serve static files with the CM anymore.
CUSTOM_TOS_HREF and CUSTOM_TOS_TEXT removed because we don't use them anywhere.

Motivation and Context

Close to the end of the work for PP-4.

How Has This Been Tested?

Running locally
Tests in CI

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-02-01T17:26:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6540ea0) 89.53% compared to head (5fa96fb) 89.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   89.53%   89.97%   +0.44%     
==========================================
  Files         245      244       -1     
  Lines       28817    28447     -370     
  Branches     6595     6493     -102     
==========================================
- Hits        25802    25596     -206     
+ Misses       2040     1895     -145     
+ Partials      975      956      -19

Flag	Coverage Δ
Api	`75.45% <52.80%> (+0.16%)`	⬆️
Core	`60.04% <92.95%> (+0.72%)`	⬆️
migration	`27.04% <32.39%> (+0.40%)`	⬆️

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.

jonathangreen · 2024-02-01T20:44:13Z

api/app.py

+        os.environ["PALACE_SECRET_KEY"] = "".join(
+            secrets.choice(string.ascii_lowercase) for i in range(24)
+        )
+


This is in the debug run function. Just allows us to run a debug environment with app.py without having to specify a SECRET_KEY every time.

Minor: Might be useful to clarify in the comment that this is only safe because this is a debug environment.

This one will go away when #1664 goes in

core/util/notifications.py

docker/startup/01_set_simplified_environment.sh

tests/api/test_controller_cm.py

jonathangreen · 2024-02-01T20:54:31Z

This one is ready for review now, but I'm leaving it in draft until the additional PR in the description are ready to go, so it doesn't get merged early.

tdilauro

Looks good. A few questions/comments, but no show stoppers.

tdilauro · 2024-02-06T18:54:12Z

README.md

+#### General
+
+- `PALACE_BASE_URL`: The base URL of the application. Used to create absolute links. (optional)
+- `PALACE_SECRET_KEY`: A secret key used for used to provide cryptographic signing, and should be set to a unique,


duplicated words: key [used for] [used to] provide

Removed this part of the README, in favor of the work in #1664

tdilauro · 2024-02-06T19:04:31Z

api/admin/controller/view.py

+                sitewide_tos_href=Configuration.DEFAULT_TOS_HREF,
+                sitewide_tos_text=Configuration.DEFAULT_TOS_TEXT,


Should these be configurable, in case a non-Palace entity wants to run this code?

When we were running the Library Simplified code, we never actually set these did we? It wasn't until Palace when we customized them. I was thinking that we can make this configurable if/when someone needs it.

Happy to make them env vars though, if you disagree.

tdilauro · 2024-02-06T19:06:46Z

api/app.py

@@ -61,12 +59,12 @@ def get_locale():
 PalaceXrayProfiler.configure(app)


-def initialize_admin(_db=None):
+def initialize_admin(_db: Session | None = None, secret_key: str | None = None):


The secret key is required. Should we allow a None here?

This went away with the work in #1664

tdilauro · 2024-02-06T19:10:38Z

api/app.py

+        os.environ["PALACE_SECRET_KEY"] = "".join(
+            secrets.choice(string.ascii_lowercase) for i in range(24)
+        )
+


Minor: Might be useful to clarify in the comment that this is only safe because this is a debug environment.

tdilauro · 2024-02-06T19:22:16Z

api/config.py

-        {
-            "key": CUSTOM_TOS_HREF,
-            "label": _("Custom Terms of Service link"),
-            "required": False,
-            "default": DEFAULT_TOS_HREF,
-            "description": _(
-                "If your inclusion in the SimplyE mobile app is governed by terms other than the default, put the URL to those terms in this link so that librarians will have access to them. This URL will be used for all libraries on this circulation manager."
-            ),
-        },
-        {
-            "key": CUSTOM_TOS_TEXT,
-            "label": _("Custom Terms of Service link text"),
-            "required": False,
-            "default": DEFAULT_TOS_TEXT,
-            "description": _(
-                "Custom text for the Terms of Service link in the footer of these administrative interface pages. This is primarily useful if you're not connecting this circulation manager to the SimplyE mobile app. This text will be used for all libraries on this circulation manager."
-            ),
-        },


Should these get mapped to PALACE_ env vars, in case a non-palace entity wants to run the code? We could still use the default href and/or text, if they were not specified.

tdilauro · 2024-02-06T19:42:53Z

core/monitor.py

-                )
-            )
-            return
-        return super(ReaperMonitor, self).run()


Some lingering Python 2 goodness.

tdilauro · 2024-02-06T20:51:29Z

tests/core/test_holds_notifications.py

-    def test_script_run(self, holds_fixture: HoldsNotificationFixture):
-        db = holds_fixture.db
-        patron1 = db.patron()
-        work1 = db.work(with_license_pool=True)
-        work2 = db.work(with_license_pool=True)
-        hold1, _ = work1.active_license_pool().on_hold_to(patron1, position=0)
-        hold2, _ = work2.active_license_pool().on_hold_to(patron1, position=0)
-
-        with patch("core.jobs.holds_notification.PushNotifications") as mock_notf:
-            holds_fixture.monitor.run()
-            assert mock_notf.send_holds_notifications.call_count == 1
-            assert mock_notf.send_holds_notifications.call_args_list == [
-                call([hold1, hold2])
-            ]
-
-            # Sitewide notifications are turned off
-            mock_notf.send_holds_notifications.reset_mock()
-            ConfigurationSetting.sitewide(
-                db.session, Configuration.PUSH_NOTIFICATIONS_STATUS
-            ).value = ConfigurationConstants.FALSE
-            holds_fixture.monitor.run()
-            assert mock_notf.send_holds_notifications.call_count == 0


Do we want to get rid of this whole test or just the part related to the defunct Configuration.PUSH_NOTIFICATIONS_STATUS?

Good call. Added back in the non-config part of this test.

jonathangreen force-pushed the feature/sitewide-settings branch from 14d438d to 5fad772 Compare February 1, 2024 18:04

jonathangreen commented Feb 1, 2024

View reviewed changes

jonathangreen requested a review from a team February 1, 2024 20:53

jonathangreen force-pushed the feature/sitewide-settings branch from f70e6a0 to 7b6a693 Compare February 1, 2024 21:08

jonathangreen mentioned this pull request Feb 2, 2024

Remove sitewide settings configuration (PP-494) ThePalaceProject/circulation-admin#101

Merged

2 tasks

tdilauro self-requested a review February 6, 2024 18:48

tdilauro approved these changes Feb 6, 2024

View reviewed changes

jonathangreen mentioned this pull request Feb 7, 2024

Add a new key table to hold some of our sitewide keys (PP-893) #1664

Merged

2 tasks

jonathangreen force-pushed the feature/sitewide-settings branch from 49c4897 to 7b6a693 Compare February 7, 2024 23:31

jonathangreen changed the base branch from main to feature/keys-table February 8, 2024 01:25

jonathangreen force-pushed the feature/sitewide-settings branch from 7b6a693 to dd46877 Compare February 8, 2024 01:25

jonathangreen force-pushed the feature/keys-table branch from 4f2bc3e to 29a4be4 Compare February 8, 2024 14:22

jonathangreen force-pushed the feature/sitewide-settings branch from 5212a4f to 8feade0 Compare February 8, 2024 14:25

Base automatically changed from feature/keys-table to main February 8, 2024 18:07

jonathangreen added 7 commits February 8, 2024 14:23

Replace sitewide settings with service configuration.

3d209f9

Remove unused settings controller

95d43ad

Add some documentation

144028f

Fix globbing issue in simplified env var script

a5c1196

Remove bearer token key, and use secret key instead.

9284b06

Remove secret key env var settings

2f053f4

Add back in TestHoldsNotifications test.

d4fb355

jonathangreen force-pushed the feature/sitewide-settings branch from 0a60402 to d4fb355 Compare February 8, 2024 18:23

jonathangreen added UI Update This PR requires an admin UI update Deployment Changes Requires changes to deployment configuration. labels Feb 8, 2024

Bump up admin ui version.

5fa96fb

jonathangreen marked this pull request as ready for review February 8, 2024 19:48

jonathangreen merged commit 20bdf4a into main Feb 8, 2024
28 checks passed

jonathangreen deleted the feature/sitewide-settings branch February 8, 2024 19:56

jonathangreen added the incompatible changes Changes that require a new major version label Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sitewide settings to environment variables (PP-494) #1648

Move sitewide settings to environment variables (PP-494) #1648

jonathangreen commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

jonathangreen Feb 1, 2024

tdilauro Feb 6, 2024

jonathangreen Feb 8, 2024

jonathangreen commented Feb 1, 2024

tdilauro left a comment

tdilauro Feb 6, 2024

jonathangreen Feb 8, 2024

tdilauro Feb 6, 2024

jonathangreen Feb 8, 2024

tdilauro Feb 6, 2024

jonathangreen Feb 8, 2024

tdilauro Feb 6, 2024

tdilauro Feb 6, 2024

tdilauro Feb 6, 2024

tdilauro Feb 6, 2024

jonathangreen Feb 8, 2024

		sitewide_tos_href=Configuration.DEFAULT_TOS_HREF,
		sitewide_tos_text=Configuration.DEFAULT_TOS_TEXT,

Move sitewide settings to environment variables (PP-494) #1648

Move sitewide settings to environment variables (PP-494) #1648

Conversation

jonathangreen commented Feb 1, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Feb 1, 2024

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading