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

Support free flow text for assigned_facility in Shifting form #1246

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

Ashesh3
Copy link
Member

@Ashesh3 Ashesh3 commented Apr 12, 2023

Required for: ohcnetwork/care_fe#5323

Proposed Changes

  • Add assigned_facility_external to store the raw name of facility which are not in CARE. This is used to support free flow input in theShifting form for Assigned facility.
  • preferred_vehicle_choice is made optional, because it is being hid from the frontend.

PR is split into two commits, changes and formatting

@coronasafe/code-reviewers

elif validated_data.get("assigned_facility"):
validated_data["assigned_facility_external"] = None

if validated_data.get("assigned_facility"):
Copy link
Member

Choose a reason for hiding this comment

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

This condition is just above it as well right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The above one is for the update method, this one is in the create method.

Copy link
Member

Choose a reason for hiding this comment

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

Line 262 and 259 are the same condition right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep those two are the same.. I had separated those code blocks as they served different purposes. The purpose of the initial if else block was to clear out the "other" facility source, so that both external and care facilities could not be set together.

The second if block was to set the value of assigned_facility_id (old code, already present).

I can proceed with merging those two conditions. So, it'll look like: (Basically merge those two if blocks)

        if validated_data.get("assigned_facility_external"):
            validated_data["assigned_facility"] = None
        elif validated_data.get("assigned_facility"):
            validated_data["assigned_facility_external"] = None
            assigned_facility_external_id = validated_data.pop("assigned_facility")[
                "external_id"
            ]
            if assigned_facility_external_id:

                validated_data["assigned_facility_id"] = Facility.objects.get(
                    external_id=assigned_facility_external_id
                ).id

Should I go ahead with this change?

care/facility/api/serializers/shifting.py Outdated Show resolved Hide resolved
care/facility/api/serializers/shifting.py Outdated Show resolved Hide resolved
if assigned_facility_external_id:
validated_data["assigned_facility_id"] = Facility.objects.get(
external_id=assigned_facility_external_id
).id
assigned = True
if validated_data.get("assigned_facility_external"):
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what is happening in this block

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are assigning an external facility, there's is no data about it in the care platform, hence assigned_facility_id and assigned_facility_object cannot be set in this case. So, this block removes those. The only information we have about this external facility is its name. No id or facility_object is available to set. So they are set to None

@sonarcloud
Copy link

sonarcloud bot commented Apr 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 52.38% and project coverage change: -0.05 ⚠️

Comparison is base (04ea55b) 56.33% compared to head (f1de376) 56.28%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1246      +/-   ##
==========================================
- Coverage   56.33%   56.28%   -0.05%     
==========================================
  Files         195      195              
  Lines        9554     9565      +11     
  Branches     1603     1606       +3     
==========================================
+ Hits         5382     5384       +2     
- Misses       4117     4126       +9     
  Partials       55       55              
Impacted Files Coverage Δ
care/facility/api/serializers/shifting.py 50.00% <23.07%> (-3.28%) ⬇️
care/facility/models/shifting.py 77.19% <100.00%> (+0.40%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gigincg gigincg merged commit 6650b79 into ohcnetwork:master Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants