AB#259554: Sync of Offices should trigger Programme sync

[sergey-misuk-valor]

AB#259554

[domdinicola]

@sergey-misuk-valor pls lint, test, and conflicts

[codecov]

Codecov Report

❌ Patch coverage is 96.92833% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.61%. Comparing base (92f2121) to head (8eea531).

Files with missing lines	Patch %	Lines
src/country_workspace/contrib/hope/sync/base.py	92.47%	6 Missing and 1 partial ⚠️
...country_workspace/contrib/hope/sync/context_geo.py	95.91%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #153      +/-   ##
===========================================
+ Coverage    95.53%   95.61%   +0.07%     
===========================================
  Files          181      181              
  Lines         5756     5790      +34     
  Branches       521      522       +1     
===========================================
+ Hits          5499     5536      +37     
+ Misses         156      152       -4     
- Partials       101      102       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[datamik]

@sergey-misuk-valor If i am not mistaken this will be fired on every new db connection creation and do we need to initiate cache_manager on every time new connection is created?

[sergey-misuk-valor]

@datamik

https://docs.djangoproject.com/en/5.2/ref/signals/#django.db.backends.signals.connection_created

Sent when the database wrapper makes the initial connection to the database. This is particularly useful if you’d like to send any post connection commands to the SQL backend.

it's only sent when initial connection created. But if it wasn't nothing complex or harmful happens in the init. Here I'm solving the warning issued by Django, because init implicitly imports db related code before db connection is performed

[datamik]

Yes but database wrapper initiates connection on every http request because CONN_MAX_AGE is set to 0 by default and Django closes connection at the end of the request. You can easily test it.

[vitali-yanushchyk-valor]

It seems Sergey relocated cache initialization from CacheConfig.ready() to a connection_created signal handler to suppress the “Accessing the database during app initialization” runtime warning - early DB access can break migrations, etc. I suspect this warning is triggered by Constance reading its settings from the database in __init__(). Instead, we could leave cache_manager.init() in ready() and simply try to add:

CONSTANCE_DATABASE_CACHE_BACKEND = "default"

so that Constance’s reads go through Redis rather than the database - our default cache is Redis.

Or we can use the signal and set a non-zero CONN_MAX_AGE, but then init will depend on the connection pool, so it may never start when it needs to, or it may start in an unpredictable way - in this case additional tests will be required; with CONN_MAX_AGE = 0, it fires on every request, causing excessive re-initialization.

[sergey-misuk-valor]

@datamik

Here's the init method:

    def init(self) -> None:
        from . import handlers  # noqa: F401

        try:
            self.cache_timeout = config.CACHE_TIMEOUT
            self.cache_by_version = config.CACHE_BY_VERSION
            if self.cache_by_version:
                self.cw_version = VERSION
        except Exception as e:
            logger.exception(e)
            capture_exception(e)

It requires DB access to read cache config. Also we probably want to notice cache config change as it's handled using constance. So I think it's a good place to put this method call in a connection_created signal handler as we want to read cache config as soon as possible.

The CONN_MAX_AGE is a different story. It's our DB configuration issue. I'm not sure if it's done on purpose or not. But we can discuss it.