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

Fix edge case for middleware hostname override in asset.py #1733

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

Ashesh3
Copy link
Member

@Ashesh3 Ashesh3 commented Nov 29, 2023

This pull request addresses a subtle yet important issue in the asset management API, specifically in the operate_assets method of care/facility/api/viewsets/asset.py and the check_asset_status function in care/facility/tasks/asset_monitor.py. The modification ensures a more reliable fallback mechanism for retrieving the middleware hostname associated with assets.

Previously, the code utilized the asset.meta.get("middleware_hostname", asset.current_location.middleware_address) pattern. This approach, however, did not account for scenarios where asset.meta['middleware_hostname'] could be an empty string. In such cases, the intended fallback to asset.current_location.middleware_address was bypassed, inadvertently defaulting to asset.current_location.facility.middleware_address due to the outer or condition.

The current update addresses this issue by repositioning the fallback logic. Instead of relying on the get method's default value, the code now explicitly checks for the truthiness of the middleware_hostname retrieved from asset.meta. This change ensures that if middleware_hostname is an empty string, the code will correctly fall back to asset.current_location.middleware_address, and subsequently to asset.current_location.facility.middleware_address if needed.

@coronasafe/care-backend-maintainers @coronasafe/care-backend-admins

@Ashesh3 Ashesh3 added the P1 High priority; urgent label Nov 29, 2023
@nihal467
Copy link
Member

LGTM ,

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f287b46) 60.76% compared to head (abf5523) 60.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1733   +/-   ##
=======================================
  Coverage   60.76%   60.76%           
=======================================
  Files         210      210           
  Lines       11530    11530           
  Branches     1642     1642           
=======================================
  Hits         7006     7006           
  Misses       4283     4283           
  Partials      241      241           

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

@vigneshhari vigneshhari merged commit cae1743 into master Nov 30, 2023
7 checks passed
@vigneshhari vigneshhari deleted the middleware-override-fix branch November 30, 2023 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High priority; urgent waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants