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 crash when feature's id is missing from feature_types #3553

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

Ashesh3
Copy link
Member

@Ashesh3 Ashesh3 commented Sep 7, 2022

Fix #3552

The search results page crashes when a facility has a feature whose id is not mapped in the FACILITY_FEATURE_TYPES. This PR adds a safe check for existence before accessing the feature's properties.

image

@Ashesh3 Ashesh3 requested a review from a team September 7, 2022 07:06
@Ashesh3 Ashesh3 requested a review from a team as a code owner September 7, 2022 07:06
@Ashesh3 Ashesh3 requested a review from bodhish September 7, 2022 07:06
@netlify
Copy link

netlify bot commented Sep 7, 2022

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 467dbd3
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/63184c19894af0000aa825af
😎 Deploy Preview https://deploy-preview-3553--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@nihal467
Copy link
Member

nihal467 commented Sep 7, 2022

@Ashesh3 https://deploy-preview-3553--care-net.netlify.app/facility/dbfac015-0556-4450-8435-20bf3d97bc14

now, the search got ready, but when the facility button is clicked, it is crashing there, can you check indetail

@sonarcloud
Copy link

sonarcloud bot commented Sep 7, 2022

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
1.1% 1.1% Duplication

@Ashesh3
Copy link
Member Author

Ashesh3 commented Sep 7, 2022

@nihal467 Should be fixed now. Somehow the facility "test-nihal-001" has a feature which doesn't exist (ID 6). This shouldn't be possible as the frontend has no option to set a feature with ID 6 during asset creation. Maybe there needs to be checks on the backend to disallow such invalid values.

@khavinshankar
Copy link
Member

@Ashesh3 can you please raise an issue about this in the backend?

@Ashesh3
Copy link
Member Author

Ashesh3 commented Sep 7, 2022

@Ashesh3 can you please raise an issue about this in the backend?

Sure!

@Ashesh3
Copy link
Member Author

Ashesh3 commented Sep 7, 2022

@khavinshankar The backend does have proper validation for feature ids.

This issue seemed to be due to an inconsistency between the features mapping on the backend and frontend.

Backend:

 FEATURE_CHOICES = [
    (1, "CT Scan Facility"),
    (2, "Maternity Care"),
    (3, "X-Ray facility"),
    (4, "Neonatal care"),
    (5, "Operation theater"),
    (6, "Blood Bank"),
]

Frontend:

export const FACILITY_FEATURE_TYPES = [
  {
    id: 1,
    name: "CT Scan",
    icon: "circle-dot",
  },
  {
    id: 2,
    name: "Maternity Care",
    icon: "person-breastfeeding",
  },
  {
    id: 3,
    name: "X-Ray",
    icon: "x-ray",
  },
  {
    id: 4,
    name: "Neonatal Care",
    icon: "baby",
  },
  {
    id: 5,
    name: "Operation Theater",
    icon: "syringe",
  },
];

Feature "Blood Bank" with id 6 has not been added to the frontend yet.

Issue (Addition of Blood Bank): #3469

The below two PRs ideally should have been merged together:
Backend change: coronasafe/care#993
Frontend change: #3474

But it seems that the backend change was merged but the frontend lagged behind, which caused this issue.

@nihal467
Copy link
Member

nihal467 commented Sep 7, 2022

@Ashesh3 @khavinshankar it is working now, do we need this PR or close it?

@Ashesh3
Copy link
Member Author

Ashesh3 commented Sep 7, 2022

@Ashesh3 @khavinshankar it is working now, do we need this PR or close it?

We still need this PR to fix it, This adds a conditional check to not print attributes which do not exist. This should make the care platform more robust and crash proof. This will ensure that crashes do not happen in future even if backend and frontend are not in sync (which is the case currently even now).

@skks1212 skks1212 added the P1 breaking issue or vital feature label Sep 7, 2022
@gigincg gigincg merged commit c64ade1 into coronasafe:develop Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing P1 breaking issue or vital feature urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug : Facility Search is not working
5 participants