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

FOUR-6601: Refactor settings #4479

Merged
merged 23 commits into from
Sep 2, 2022
Merged

FOUR-6601: Refactor settings #4479

merged 23 commits into from
Sep 2, 2022

Conversation

runyan-co
Copy link
Contributor

@runyan-co runyan-co commented Aug 16, 2022

Environment Setup

Issue & Reproduction Steps

We used to utilize a tagged Redis cache to store settings to reduce the number of queries to the database every time the app is loaded. Over time, we've noticed the cache actually creates too many network calls to Redis (it makes a network call everytime a cached setting is called) and it uses a lot more memory by each PHP process. We also had a settings() function helper which complicates things as all settings were also in the global app config (accessed using the config() function helper).

This PR refactors the settings so they can be easily accessed through the global app configuration. This will reduce the duplicative nature of the settings() helper and simplify the app config caching process.

Solution

The SettingsServiceProvider loads all settings from the database and binds the key and config values into the app’s global configuration. If the app configuration is cached when a setting is saved or updated, the config will automatically be cleared and re-cached. Horizon will also be restarted when the configuration is re-cached to ensure it picks up the updated config values.

Refactoring settings usage in packages

Multiple packages implement and use settings, however this refactor deprecates the settings(), flush_settings(), and the cache_settings() function helpers. There are also other changes which need to be made to specific place in the below packages to ensure the settings are correctly implemented.

How to Test

Extensive testing will be required to ensure all PRs cover all use cases of settings, ensuring they are updated (even when the configuration is cached).

Related Tickets & Packages

Code Review Checklist

  • I have pulled this code locally and tested it on my instance, along with any associated packages.
  • This code adheres to ProcessMaker Coding Guidelines.
  • This code includes a unit test or an E2E test that tests its functionality, or is covered by an existing test.
  • This solution fixes the bug reported in the original ticket.
  • This solution does not alter the expected output of a component in a way that would break existing Processes.
  • This solution does not implement any breaking changes that would invalidate documentation or cause existing Processes to fail.
  • This solution has been tested with enterprise packages that rely on its functionality and does not introduce bugs in those packages.
  • This code does not duplicate functionality that already exists in the framework or in ProcessMaker.
  • This ticket conforms to the PRD associated with this part of ProcessMaker.

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For saved search I just did a test, and it seems the settings still cached, please watch the following video @runyan-co

Screen.Recording.2022-08-19.at.10.18.42.mov

To make the changes take effect you need to restart horizon manually:

Screen.Recording.2022-08-19.at.10.22.26.mov

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extended properties configuration working correctly:

Screen.Recording.2022-08-19.at.10.37.29.mov

SSO configuration working correctly:

Screen.Recording.2022-08-19.at.10.55.04.mov

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Horizon is terminating constantly after trying to run a process with data connector:

Screen.Recording.2022-08-19.at.10.57.11.mov
Screen.Recording.2022-08-19.at.10.59.41.mov

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABE seems to be working correctly:

Screen.Recording.2022-08-19.at.12.54.28.mov

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCIM settings working correctly:

Screen.Recording.2022-08-19.at.16.32.57.mov

Copy link
Contributor

@agustinbusso agustinbusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SAML settings working correctly:

Screen.Recording.2022-08-19.at.16.58.15.mov

@runyan-co
Copy link
Contributor Author

runyan-co commented Aug 23, 2022

Details

Hey @agustinbusso, I reviewed your screen recording and it looks like it's working correctly. Since you're running horizon in the foreground (using php artisan horizon), it's sending a shutdown signal to that particular PHP process. In a production environment (or my local dev environment, cause I'm lazy), supervisor is usually used to continually spawn new PHP processes (running horizon) as needed.

If you're interested, I included the supervisor config file I use in my local environment. You'd need to change out the path to artisan in the file and then add the file to the supervisor config directory:/usr/local/etc/supervisor.d/horizon.conf (or /opt/homebrew/etc/supervisor.d/horizon.conf for M1 MacBooks). Then restart supervisor (supervisorctl reread, supervisorctl update, supervisorctl restart all) and it should run horizon in the background for you.

[program:horizon]
process_name=%(program_name)s_%(process_num)02d
command=/opt/homebrew/bin/php /Users/alex/code/processmaker/artisan horizon
autostart=true
autorestart=true
stopasgroup=true
killasgroup=true
user=alex
numprocs=1
redirect_stderr=true
stdout_logfile=/Users/alex/logs/%(program_name)s_%(process_num)02d.log
stopwaitsecs=3600

@agustinbusso
Copy link
Contributor

Saved search send email report working correctly

Screen.Recording.2022-08-24.at.10.52.27.mov

@agustinbusso
Copy link
Contributor

Send email from connector working correctly:

Screen.Recording.2022-08-24.at.11.17.06.mov

@agustinbusso
Copy link
Contributor

Docusign settings changing correctly:

Screen.Recording.2022-08-24.at.11.34.45.mov

@agustinbusso
Copy link
Contributor

Test for FOUR-6599:
Send Report of save search is sent without having configurations in the email settings

Working well, settings are not cached:

Screen.Recording.2022-08-24.at.15.29.33.mov
Screen.Recording.2022-08-24.at.15.27.55.mov

@nolanpro nolanpro self-requested a review August 26, 2022 18:40
@danloa
Copy link
Contributor

danloa commented Aug 30, 2022

Check list:
Configure email - OK
Test email connector - OK
Configure ABE - OK
Test ABE - OK
Collections - OK
Configure Ethos - OK
Ethos - OK

@ryancooley ryancooley merged commit a85ad49 into develop Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants