T3348-Fix-failing-quality_test_Global_Childpool - #2124
Open
loris-fab wants to merge 5 commits into
Open
Conversation
- do_365_mix() was calling the GMC search API sequentially once per day of the year (~1m30 total), which looked like an infinite loading spinner to users. Requests are now fired concurrently via a thread pool and results are re-sorted chronologically afterwards. - Add a confirmation popup on the "365 children" button to set expectations on the search duration.
There was a problem hiding this comment.
Pull request overview
This PR addresses a perceived “infinite loading” issue on the Global Childpool “365 children” action by parallelizing daily GMC API calls and improving UX with a confirmation dialog, while also fixing correctness issues in the prior sequential logic.
Changes:
- Reworked
do_365_mix()to prepare per-day requests and execute them concurrently viaThreadPoolExecutor, then re-sort results chronologically. - Fixed correctness issues around pagination (
skip) and thenb_foundcounter by resettingskipand summingNumberOfBeneficiaries. - Added a confirmation popup on the “365 children” button to set user expectations before starting the long-running search.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| child_compassion/wizards/global_child_search.py | Parallelizes the 365/366-day GMC search and adjusts result aggregation/state updates. |
| child_compassion/views/global_childpool_view.xml | Adds a user confirmation prompt before launching the “365 children” search. |
Suppressed comments (3)
child_compassion/wizards/global_child_search.py:368
future.result()will raise if any HTTP call errors, which will abort the entire 365-day search and leave the wizard in a partially-reset state. It’s safer to handle exceptions per-future and record the date as missing (or surface a summarized error) rather than crashing the whole run.
with concurrent.futures.ThreadPoolExecutor(max_workers=15) as executor:
futures = [executor.submit(fetch_http, req) for req in prepared_requests]
for future in concurrent.futures.as_completed(futures):
results.append(future.result())
child_compassion/wizards/global_child_search.py:380
OnrampConnector.send_message()setsresult["content"]to a string when the response isn’t valid JSON. In that case,result.get("content", {}).get(...)will raise because strings don’t have.get(), breaking the whole loop. Guard the type before treatingcontentas a dict.
for c_date, result in results:
if result.get("code") == 200 and result.get("content", {}).get(result_name):
total_matching_found += result["content"].get(
"NumberOfBeneficiaries", 0
)
children_data = result["content"][result_name]
for child_data in children_data:
child_compassion/wizards/global_child_search.py:317
missing_datesis only written at the very end of the method. If an exception occurs before the finalwrite()(for example during concurrent requests), the wizard can keep stalemissing_datesfrom a previous run while already having deletedglobal_child_ids. Reset it up-front to keep the wizard state consistent even on failure.
self.global_child_ids.unlink()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Confidence Score: 5/5
What T-Rex did
Reviews (4): Last reviewed commit: "[T3348] FIX: keep the auth headers when ..." | Re-trigger Greptile |
- Ensure thread safety by instantiating OnrampConnector inside worker thread. - Fix error handling: raise UserError on non-200/failed API calls instead of adding them to missing_dates. - Update docstring for leap year handling.
- OnrampConnector is a process-wide singleton whose __new__ (cold start) and __init__ (token refresh) both read res.config.settings through the Odoo env, so calling it from a ThreadPoolExecutor worker used the request's cursor concurrently. - The resulting exception was swallowed by the worker and turned into a None result, making the whole 365 search fail with a UserError. - The connector is now built once on the request thread; workers only call send_message(), which never touches the ORM.
…a ConnError - send_message() rebuilt its requests.Session on a connection error while restoring only the params, dropping the OAuth bearer header set by _retrieve_token: the retry and every later call went unauthenticated. - The session is shared by the singleton, so a single transient network failure poisoned all subsequent GMC calls, e.g. the parallel 365 search which would then fail with a UserError. - The replacement session now inherits the headers of the broken one and is stored on the class, like __new__ does, instead of shadowing it with an instance attribute.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Clicking the "365 children" button on the Global Childpool search page appeared to hang indefinitely (frozen screen, endless spinner), reported as a functional bug.
The feature wasn't actually broken: it ran 365 sequential HTTP requests to the GMC API (one per day of the year), taking ~1m30. Making them concurrent then exposed two latent defects in the shared GMC connector, fixed
here as well.
Technical aspect
do_365_mix()(wizards/global_child_search.py) now fires the 365 (366 on leap years) requests concurrently via aThreadPoolExecutor(15 workers), then re-sorts the results chronologically before creating thecompassion.global.childrecords.add_search()didself.skip += self.take(0 + 1), so every date from Jan 2nd onwards asked for the second best match. Dates with a single matching child returned nothing andwere wrongly listed in
missing_dates.skipis now0for all requests.nb_found: it was overwritten on each iteration, so it only reflected December 31st. It is now the sum over all successful daily responses.UserErrorwithout distinguishing "no child for that date" from an API error, so GMC failures were silently shown as "Missing birthdates". Only empty 200 responses are listedas missing now; real errors surface the GMC message.
OnrampConnectorthread-safety (message_center_compassion, shared by every GMC call in the repo):res.config.settingsthrough the request's DB cursor. Building it inside the workers used that cursor concurrently. It is now instantiated once on therequest thread; workers only call
send_message(), which never touches the ORM.ConnError,send_message()rebuilt itsrequests.Sessionrestoring only the params, dropping the bearer token. As the session is process-wide, one network drop left every later GMC callunauthenticated. The new session now inherits the headers too.
views/global_childpool_view.xml) to set expectations on the search duration. Now it take 10 s instead of 1m30.Misc
message_center_compassion: the connector changes are corrective (no API change) but affect all outbound GMC calls. taOutcome
The end result is a 365 children button that works 10 times faster.