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

added simple root Event and fixed OGBillOfLading Generator #164

Merged
merged 15 commits into from
Jun 29, 2021
Merged

added simple root Event and fixed OGBillOfLading Generator #164

merged 15 commits into from
Jun 29, 2021

Conversation

mkhraisha
Copy link
Collaborator

Adds a base "Event" Schema that captures very simple information.

This schema should be extended by the following EPCIS event types:

  • observation
  • transformation
  • aggregation
  • disaggregation
  • commission
  • decommission

This PR is meant to spark conversation on what fields need to be in the base Event vs one of the Event Types

@mkhraisha mkhraisha requested review from mprorock and OR13 as code owners May 17, 2021 17:25
Copy link
Collaborator

@mprorock mprorock left a comment

Choose a reason for hiding this comment

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

Awesome start on this - I think the Event is going to be a a key interop point, and we will certianly need to issue VCs against certain events at least in our case on the ag side of things

"description": "The organization performing the activity.",
"type": "array",
"items": {
"$ref": "https://w3id.org/traceability/schemas/Organization.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want this to be an org? or is entity possibly better? We at least also deal with individuals in the supply chain where Person sometimes is a better representation than Organization

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had misgivings on whether we should have two fields or just one for this, I wanted initially to have an actor and an actingOrg field.

An entity might solve the problem. good shout

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer use of Entity

"description": "Time when the event took place",
"type": "string"
},
"products": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am going to have to mull over use of Product here. I think it is probably correct, but also thinking about raw materials becoming a product etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

see note from @OR13 below - should this be GS1 or schema.org? Other aspects of the vocab are utilizing schema.org

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider "inputs" (raw materials, outputs from other processes/sources, etc.) and "outputs" (refined version of inputs becoming inputs to other processes/consumers, finished products, etc.). These can then be easily turned into a complex trace from most-basic-origin (e.g., extraction of various ores from deep underground) to most-refined-consumable (e.g., a smartphone, tablet, or computer)...

"description": "Location where event took place",
"$ref": "https://w3id.org/traceability/schemas/Place.json"
},
"eventTime": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may want to actually record a few more things here (possibly optionally), e.g. scheduled or planned start time, planned duration, actual start time, actual end time, etc. open on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer ability to record at a minimum

  1. event start time
  2. event end time

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this makes sense to split up, probably another base object that encapsulates eventTime that would have the planned start/duration etc.

@OR13
Copy link
Collaborator

OR13 commented May 18, 2021

@mprorock can you clearly state what changes need to be made in order to accept this PR?

potentially we can open issues to track them and iterate?

}
},
"eventLocation": {
"$comment": "{\"term\": \"eventLocation\", \"@id\": \"https://w3id.org/traceability#eventLocation\"}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we want to be defining "eventLocation"... can we use an existing ontology for this field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 I think this should be a Place

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

absolutely should be place

Copy link
Collaborator

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

https://github.com/w3c-ccg/traceability-vocab/pull/164/files#r634604678

^ this is the only critical objection I have, we need to be cleaning up our examples and avoid context nesting.

@mkhraisha
Copy link
Collaborator Author

Removed child context @mprorock i deleted the generator and addressed most things in this conversation except for the event time stuff. would it be ok if i open an issue and assign it to you?

@mprorock
Copy link
Collaborator

event time stuff

let's not worry about the event time stuff as a blocker for now - we can add that as a later detail when required - probably as a subclass of this (e.g. event with duration or something like that)

@OR13 OR13 self-requested a review June 1, 2021 17:38
@OR13
Copy link
Collaborator

OR13 commented Jun 1, 2021

On the call we discussed giving this one for day for reviews.

Copy link
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Made many description strings more consistent with each other (start with article, close with period, etc.), particularly in ontology. Some sample data is not good English (sentence fragments, primarily) but I'm not always sure what was intended, so some are now unchanged — but will want followup attention.

packages/traceability-schemas/schemas/Event.json Outdated Show resolved Hide resolved
packages/traceability-schemas/schemas/Event.json Outdated Show resolved Hide resolved
packages/traceability-schemas/schemas/Event.json Outdated Show resolved Hide resolved
packages/traceability-schemas/schemas/Event.json Outdated Show resolved Hide resolved
packages/traceability-schemas/schemas/Event.json Outdated Show resolved Hide resolved
@mkhraisha
Copy link
Collaborator Author

The remaining suggestions modify generated text, we probably need to actually dive into the generators and figure out how to fix them. This should probably be made its own issue and tracked accordingly. @OR13 @mprorock @TallTed

@OR13
Copy link
Collaborator

OR13 commented Jun 8, 2021

These proposed changed look good to me, @mkhraisha can you approve them / reject them so we can see where any gaps are remaining?

@OR13
Copy link
Collaborator

OR13 commented Jun 15, 2021

Feels like the generators may be getting in the way of having "realistic fixtures"... we should continue to discuss this on the related open issues.

@TallTed
Copy link
Contributor

TallTed commented Jun 15, 2021

Generated files should generally not be in git repos. Whatever can be done to shift things so that the generated files go to github.io or similar would be best.

@mkhraisha
Copy link
Collaborator Author

Apologies for the delays in tackling this.
Should be all good to go now.

@OR13
Copy link
Collaborator

OR13 commented Jun 21, 2021

This looks good to me.

@mprorock mprorock merged commit af88dad into w3c-ccg:main Jun 29, 2021
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

Successfully merging this pull request may close these issues.

4 participants