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

HKClinicalRecord Flagged for Review #113

Closed
bangonkali opened this issue Dec 13, 2019 · 11 comments
Closed

HKClinicalRecord Flagged for Review #113

bangonkali opened this issue Dec 13, 2019 · 11 comments
Milestone

Comments

@bangonkali
Copy link

Issue is related to #103 and #104

For some reason, even we have not enabled Clinical Records following instructions on Readme.md this snippet of code is included in our build.

if ([sample isKindOfClass:[HKClinicalRecord class]]) {
HKClinicalRecord *clinicalRecord = (HKClinicalRecord *) sample;
NSError *err = nil;
NSDictionary *fhirData = [NSJSONSerialization JSONObjectWithData:clinicalRecord.FHIRResource.data options:NSJSONReadingMutableContainers error:&err];

We have not enabled Clinical Record but Apple has flagged our product during review because of references to these code.

@bangonkali
Copy link
Author

Can we make the addition of this file generated/compile-time optional? So that there would never be references in code if it's added using Ionic 3 Cordova?

@bangonkali
Copy link
Author

bangonkali commented Dec 13, 2019

We have not enabled Clinical Record as discussed in this portion of readme.
image

@Naveen1612
Copy link

We are also facing the same issue.

@rossmartin
Copy link
Collaborator

I apologize that you both are experiencing this. When you do not provide the CLINICAL_READ_PERMISSION the plugin should not include the HealthKitClinicalRecords.h and HealthKitClinicalRecords.m files. Can you confirm that you do not see those in your Xcode project after the plugin installs? If your project has those files this is certainly the main cause of the problem.

I separated the implementation for the clinical records to avoid this issue. If you don't have the above files in your project I have a suspicion is that Apple is seeing the clinical record related code in HealthKit.m (within the getHKObjectType and getHKSampleType functions ) -

    if (@available(iOS 12.0, *)) {
      type = [HKObjectType clinicalTypeForIdentifier:elem];
      if (type != nil) {
        return type;
      }
    }

I don't think the clinical record related code depends on that anymore but I'd have to take a look further.

I have been insanely busy recently and I'm sorry but I might not be able to give much attention to this right now. I closed on a house I bought in the last week and the day job is busy - combine this with the holidays and its crazy 🎅

As a temporary workaround you can use version 0.5.5 of the plugin. This is the last version before any clinical records functionality was added.

@bangonkali
Copy link
Author

Hi @rossmartin , any update on this so far? If you could give a hint on where to start maybe we can also help out with this. ☺

@rossmartin
Copy link
Collaborator

rossmartin commented Mar 2, 2020

@bangonkali I took some time tonight to look into this issue. I created a new cordova project and installed this plugin without providing the CLINICAL_READ_PERMISSION at install. I can confirm that the Xcode project does not have the HealthKitClinicalRecords.h and HealthKitClinicalRecords.m files when the CLINICAL_READ_PERMISSION is not provided. My suspicion about the rejection from Apple is due to either -

  1. The clinical record related code in HealthKit.m (within the getHKObjectType and getHKSampleType functions ) -
    if (@available(iOS 12.0, *)) {
      type = [HKObjectType clinicalTypeForIdentifier:elem];
      if (type != nil) {
        return type;
      }
    }
  1. Despite the actual implementation not being included Apple is still rejecting the app submission because of the keywords "FHIR" and "Clinical Records" being in the HealthKit.m file. Ex -
/**
 * Search for a specific FHIR resource type
 *
 * @param command *CDVInvokedUrlCommand
 */
- (void)queryForClinicalRecordsWithFHIRResourceType:(CDVInvokedUrlCommand *)command {
  #ifdef HKPLUGIN_CLINICAL_RECORDS
  [HealthKitClinicalRecords queryForClinicalRecordsWithFHIRResourceType:command delegate:self.commandDelegate];
  #endif
}

I think it is because of the 2nd item above. In this case the approach I came up with to not include the actual implementation will not work. I had a feeling while doing some of this work to add the clinical functionality to this plugin that it should be an actual different plugin for the clinical functionality. In the react native ecosystem this is the case and I understand why. Right now I lean towards removing the clinical functionality from this plugin and either leaving my fork or a new plugin as the means to use clinical records in HealthKit for cordova.

@EddyVerbruggen would you be opposed to me removing the clinical functionality from the plugin and basically restoring it to the state it was in at version 0.5.5 ? I lean towards this and updating the README to reference my fork for anyone that wants to use the clinical records functionality.

I've seen this same problem in the react native ecosystem (and not related to HealthKit). Things get messy and convoluted when a library is responsible for dynamically adding/removing source code at install or compile time. I did my best at trying to avoid this issue but at this point I don't want to try more options blindly.

/edit @awatson1978 I'm tagging you here also on the issue. Right now I lean towards having the clinical records functionality on my fork only or a separate plugin. It's a total mess to try and get this right for those that aren't using clinical records.

@bangonkali
Copy link
Author

@rossmartin I am not a fan of the way Apple is checking our App submissions but in this case, since it's Apple we're dealing with, I would support a decision to separate the Clinical Health Record as a separate plugin rather than complicating the approach for this plugin.

Users will be left to decide if they want to use this plugin or the Clinical Health Record plugin.

@rossmartin
Copy link
Collaborator

@EddyVerbruggen @awatson1978 I'm going to wait until I hear from you before making any changes on this.

@EddyVerbruggen
Copy link
Owner

@rossmartin Thanks for taking the time to thoroughly explain the issue. And I agree with you. It's probably best to revert to the old state and link to your fork for anyone needing the Clinical Health Record feature.

@rossmartin
Copy link
Collaborator

@EddyVerbruggen thank you for getting back so soon and reviewing. I went ahead and reverted the PRs with the clinical record functionality. I updated the README and bumped the plugin and npm package version to 0.7.0. The repo is ready for you to publish to npm.

Thank you again for all your open source work!

@EddyVerbruggen EddyVerbruggen added this to the 0.7.0 milestone Mar 5, 2020
@EddyVerbruggen
Copy link
Owner

Perfect, @rossmartin!

Thanks for moving forward with this so quickly.

0.7.0 has been published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants