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

Realm doesn't check for optional=false objects, we have to do that in our code. #1142

Closed
petmongrels opened this issue Oct 16, 2023 · 11 comments
Assignees

Comments

@petmongrels
Copy link
Contributor

petmongrels commented Oct 16, 2023

  • We have one to one relationship in Avni models in a few places. Without not null check we will have bad offline data with downstream failures in the app. Realm checks for primitive field works fine. We could put a check in our code in RealmProxy.create method for such relationships - while Realm fixes this bug.
  • As per realm docs -
    User-defined object types are always optional except in lists and sets...
  • We can do bugsnag notify to collect these errors.
  • Add the check optional: false if necessary to be mentioned explicity to the necessary fields in the models

How to test:

  • Make a subject type only direct assignment
  • Insert data into db such that a subject outside of user's catchment is assigned to the user
  • Sync the users device
  • We should be able to see failure on user sync itself when the individual is fetched, instead of seeing the failure when opening the individual.
  • Via playground add cases to test other fields as well or add test cases for the same.

Out of scope:

Fixing existing bad data - since sometimes as an example, say individual's address is null, it could be because individual should itself not exist within the user's device. So for fixing existing data issues we can do them as and when we face issues since the scope and severity of the same is not clear.

@petmongrels petmongrels converted this from a draft issue Oct 16, 2023
@petmongrels
Copy link
Contributor Author

Individual profile failing due to missing address lineage.

https://app.bugsnag.com/samanvay-research-and-development-foundation/avni-client/errors/6529dba76307e00008a7eb28

@mahalakshme mahalakshme moved this from New Issues to In Analysis in Avni Product Oct 17, 2023
@mahalakshme
Copy link
Contributor

@petmongrels yeah the above is because the issue is not that individual doesn't have address. The issue is that, individual in itself should not be there in the user's device since the address levels of the individual are outside of user's catchment.

@mahalakshme mahalakshme moved this from In Analysis to In Analysis Review in Avni Product Oct 18, 2023
@mahalakshme mahalakshme moved this from In Analysis Review to In Analysis in Avni Product Oct 19, 2023
@mahalakshme
Copy link
Contributor

mahalakshme commented Oct 19, 2023

@petmongrels if we do this card, for what would we apply the condition saying optional as false? Since I see for an individual both gender and address can be optional and these are the cases we have faced issues. Are there any where u think this condition will be needed on priority? There are cases like IndividualRelationGenderMapping where relation and gender cant be optional but these are more static and wont change frequently and hence change of erroring is low and the priority of this card becomes low. Let me know if u can think of any cases where doing this is important.

@petmongrels
Copy link
Contributor Author

  • Individual to SubjectType
  • Program Enrolment to Program
    etc.

Also, I think in our system address is mandatory on Individual. On server it is mandatory and I think on mobile app also.

@mahalakshme mahalakshme moved this from In Analysis to In Analysis Review in Avni Product Oct 19, 2023
@mahalakshme mahalakshme moved this from In Analysis Review to Ready in Avni Product Oct 20, 2023
@petmongrels petmongrels moved this from Ready to In Progress in Avni Product Oct 20, 2023
@petmongrels petmongrels self-assigned this Oct 20, 2023
@petmongrels
Copy link
Contributor Author

petmongrels commented Oct 25, 2023

  • Note all of the following are effectively optional as of today, so the change is to when we make something non-optional.
  • There are some which have been made explicitly optional. The following list doesn't included that.

Full List

[ 'DashboardFilter --> dashboard',
  'ConceptAnswer --> concept',
  'LocationMapping --> parent',
  'LocationMapping --> child',
  'FormMapping --> form',
  'FormElementGroup --> form',
  'FormElement --> concept',
  'FormElement --> formElementGroup',
  'Individual --> subjectType',
  'Individual --> lowestAddressLevel',
  'ProgramEnrolment --> program',
  'ProgramEnrolment --> individual',
  'Observation --> concept',
  'ProgramEncounter --> encounterType',
  'ProgramEncounter --> programEnrolment',
  'Encounter --> encounterType',
  'Encounter --> individual',
  'Checklist --> detail',
  'Checklist --> programEnrolment',
  'ChecklistItem --> detail',
  'ChecklistItem --> checklist',
  'VisitScheduleInterval --> min',
  'VisitScheduleInterval --> max',
  'VisitScheduleConfig --> encounterType',
  'VisitScheduleConfig --> interval',
  'ProgramConfig --> program',
  'Family --> lowestAddressLevel',
  'Family --> headOfFamily',
  'IndividualRelationGenderMapping --> relation',
  'IndividualRelationGenderMapping --> gender',
  'IndividualRelationshipType --> individualAIsToBRelation',
  'IndividualRelationshipType --> individualBIsToARelation',
  'IndividualRelationship --> relationship',
  'IndividualRelationship --> individualA',
  'IndividualRelationship --> individualB',
  'ChecklistItemStatus --> from',
  'ChecklistItemStatus --> to',
  'ChecklistItemDetail --> concept',
  'ChecklistItemDetail --> checklistDetail',
  'VideoTelemetric --> video',
  'IdentifierAssignment --> identifierSource',
  'GroupPrivileges --> group',
  'GroupPrivileges --> privilege',
  'GroupRole --> groupSubjectType',
  'GroupRole --> memberSubjectType',
  'GroupSubject --> groupSubject',
  'GroupSubject --> memberSubject',
  'GroupSubject --> groupRole',
  'DashboardSectionCardMapping --> dashboardSection',
  'DashboardSectionCardMapping --> card',
  'DraftSubject --> subjectType',
  'DraftSubject --> lowestAddressLevel',
  'EntityApprovalStatus --> approvalStatus',
  'GroupDashboard --> dashboard',
  'DashboardSection --> dashboard',
  'Comment --> subject',
  'Comment --> commentThread',
  'DocumentationItem --> documentation',
  'TaskStatus --> taskType',
  'Task --> taskType',
  'Task --> taskStatus',
  'DraftEncounter --> encounterType',
  'DraftEncounter --> individual',
  'SubjectProgramEligibility --> subject',
  'SubjectProgramEligibility --> program',
'Settings --> locale'

]

Making explicitly optional after checking

'Settings --> locale'

Not sure how they are used so keeping it as it is, which is that they remain optional.

'VisitScheduleInterval --> min',
'VisitScheduleInterval --> max'
'VisitScheduleConfig --> encounterType',
'VisitScheduleConfig --> interval'
'Family --> headOfFamily',

Rest all to become mandatory.

petmongrels added a commit to avniproject/avni-models that referenced this issue Oct 26, 2023
petmongrels added a commit that referenced this issue Oct 26, 2023
@petmongrels petmongrels moved this from In Progress to Code Review Ready in Avni Product Oct 26, 2023
@ashusvnath
Copy link

ashusvnath commented Oct 30, 2023

I've made my comments on commit 8d70e06.

@ashusvnath ashusvnath moved this from Code Review Ready to Code Review with Comments in Avni Product Oct 30, 2023
@petmongrels petmongrels moved this from Code Review with Comments to In Progress in Avni Product Oct 30, 2023
petmongrels added a commit to avniproject/avni-models that referenced this issue Oct 30, 2023
@petmongrels petmongrels moved this from In Progress to QA Ready in Avni Product Oct 30, 2023
petmongrels added a commit to avniproject/avni-models that referenced this issue Oct 31, 2023
petmongrels added a commit to avniproject/avni-models that referenced this issue Oct 31, 2023
…used. ProgramConfig to be removed completed in a different branch.
petmongrels added a commit that referenced this issue Oct 31, 2023
petmongrels added a commit to avniproject/avni-models that referenced this issue Oct 31, 2023
petmongrels added a commit that referenced this issue Oct 31, 2023
petmongrels added a commit that referenced this issue Oct 31, 2023
@mahalakshme
Copy link
Contributor

@petmongrels I dont see whatever mentioned in Full List(like lowestAddressLevel in Individual), made mandotary. By default, are the properties mandatory? If so I dont see 'VisitScheduleInterval --> min',max having optional

@petmongrels
Copy link
Contributor Author

petmongrels commented Nov 2, 2023 via email

@ashusvnath ashusvnath moved this from QA Ready to In QA in Avni Product Nov 6, 2023
@ashusvnath
Copy link

Testing notes:

  • Able to see sync error when address is outside catchment for a directly assigned individual
  • Wrote integration tests for to assert that creation of a Subject without subjectType and lowestAddressLevel fails

@ashusvnath
Copy link

@mahalakshme : Error screenshot as per you request

Image

@vinayvenu
Copy link
Member

vinayvenu commented Nov 8, 2023

Notes

  • All optional/mandatory fields look good
  • This will not fix issues where Individual.gender is required (we've had situations earlier where gender was expected but empty)
  • Unrelated - We don't seem to be using ProgramConfig, and therefore VisitScheduleConfig and VisitScheduleInterval. We should remove this

@mahalakshme we should add cards for 2 and 3 and fix in a later release.

@github-project-automation github-project-automation bot moved this from In QA to Done in Avni Product Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants