#322 Add GeoUnit and OrgUnit Schemas #323
Conversation
schemas/common/geounit.schema.json
Outdated
| "xdm:parentGeoUnit": { | ||
| "title": "Parent Geographical Unit.", | ||
| "$ref": "https://ns.adobe.com/xdm/common/geounit", | ||
| "description": "Parent geographical unit of the current geographical unit in the geographical heirarchy." |
schemas/common/geounit.schema.json
Outdated
| "type": "object", | ||
| "meta:extensible": true, | ||
| "title": "Geo Unit", | ||
| "description": "The geographical unit of a parent geographical unit.", |
There was a problem hiding this comment.
Why the reference to parent here? How does this relate to other geo objects? Please elaborate this in the description.
There was a problem hiding this comment.
it is explained in issue #322
There was a problem hiding this comment.
Ah, you meant put them in the description as well. Done!
schemas/common/orgunit.schema.json
Outdated
| "$schema": "http://json-schema.org/draft-06/schema#", | ||
| "type": "object", | ||
| "meta:extensible": true, | ||
| "title": "Org Unit", |
schemas/common/orgunit.schema.json
Outdated
| "type": "object", | ||
| "meta:extensible": true, | ||
| "title": "Org Unit", | ||
| "description": "The organisation unit of a parent organisation.", |
There was a problem hiding this comment.
Should be "organization", US spelling.
| @@ -80,17 +80,17 @@ | |||
| "$ref": "https://ns.adobe.com/xdm/context/pushnotificationtoken" | |||
| } | |||
| }, | |||
| "xdm:orgUnitID": { | |||
There was a problem hiding this comment.
This is a breaking change, please update the CHANGELOG.md
There was a problem hiding this comment.
but, I see "meta:status": "experimental" , still ?
There was a problem hiding this comment.
OK, breaking changes between releases. Got it.
There was a problem hiding this comment.
Please see fa2c6f4 for how to document the breaking part of the change.
There was a problem hiding this comment.
thanks. keeping in mind for future.
|
fixed build and spellings. @trieloff OK by you? |
saurabhere
force-pushed
the
add-org-geoUnit
branch
from
2bc9f66 to
d70bff7
Compare
| "type": "string", | ||
| "description": "The user-friendly name for the organizational unit." | ||
| }, | ||
| "xdm:parentOrgUnit": { |
There was a problem hiding this comment.
Can we remove the parent concepts here? There can be a separate table with this lookup data along with other data. The queries would be very hard if we keep it the way it is.
| "type": "string", | ||
| "description": "The user-friendly name for the geographical unit." | ||
| }, | ||
| "xdm:parentGeoUnit": { |
There was a problem hiding this comment.
Can we remove the parent concepts here? There can be a separate table with this lookup data along with other data. The queries would be very hard if we keep it the way it is.
|
@cdegroot-adobe While your suggestion is one way to handle parent-child relationships, what are the downsides to this current approach? It seems clearer to me w.r.t schema in mind. |
|
@saurabhere approved. Please resolve the conflict. @cdegroot-adobe I see that @saurabhere's approach to modeling the relationship is in line with @kstreeter's suggestions, so I don't think there is an issue. |
|
@saurabhere please check this #307 (comment) with @anshuman-yadav – it looks like you will need to update the Once this is done, we can merge, i.e. I'm dismissing @cdegroot-adobe's requested changes. |
trieloff
dismissed
cdegroot-adobe’s stale review
Objections are in line with editor's suggestions – not an issue.
Fixes Issue #322
Introduces new concepts OrgUnit and GeoUnits
Already being used in Profile Schema.
Needs to be included before the Profile schema is stabilized to prevent breaking changes.
To Review: @cdegroot-adobe @trieloff @lrosenthol