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

feat: Add time value type #1403

Merged
merged 62 commits into from Feb 5, 2020
Merged

feat: Add time value type #1403

merged 62 commits into from Feb 5, 2020

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Aug 28, 2019

This adds a new value type, knora-base:TimeValue, which stores an xsd:dateTime (represented as xsd:dateTimeStamp in API v2).

  • Add TimeValue to knora-base and salsah-gui.
  • Support knora-base:TimeValue in SPARQL templates:
    • in API v1
    • in API v2
  • Support time values in API v1:
    • reading
    • writing
    • bulk import
    • extended search
  • Support time values in API v2:
    • reading
    • writing
    • Gravsearch
  • Update tests.
    • ValuesResponderV1Spec
    • ValuesV1R2RSpec
    • ResourcesV1R2RSpec
    • ValuesResponderV2Spec
    • ValuesRouteV2E2ESpec
    • OntologyV2R2RSpec
      • Update RDF4J library (fixes stack overflow error).
    • OntologyResponderV2Spec
    • SearchV1R2RSpec
    • SearchRouteV2R2RSpec
    • test data returned for generated client API code
  • Add update script plugin.
  • Update docs.

Resolves #1397.
Resolves #1446.
Needs dasch-swiss/knora-ui#307.
Needs dasch-swiss/dsp-tools#12.
Needs dasch-swiss/dsp-js-lib#41.

@benjamingeer
Copy link
Author

Does StandoffTimeTag have cardinalities for valueHasTimeStamp? Could you compare it to another existing standoff class?

It does, and I did.

:StandoffTimeTag rdf:type owl:Class ;

                        rdfs:subClassOf :StandoffDataTypeTag,
                                        :TimeBase ;

                        rdfs:comment "Represents a timestamp in a TextValue"@en .


:TimeBase rdf:type owl:Class ;

              rdfs:subClassOf :ValueBase ,
                              [ rdf:type owl:Restriction ;
                                owl:onProperty :valueHasTimeStamp ;
                                owl:cardinality "1"^^xsd:nonNegativeInteger
                              ] .


:valueHasTimeStamp rdf:type owl:DatatypeProperty ;

                       rdfs:subPropertyOf :valueHas ;

                       :subjectClassConstraint :TimeBase ;

                       :objectDatatypeConstraint xsd:dateTime .

@tobiasschweizer
Copy link
Contributor

I can try and check this in the afternoon if you want

@benjamingeer
Copy link
Author

Yes please. I based the mapping element on the existing one for the date tag, and I can't find anything that looks different.

@benjamingeer
Copy link
Author

Found the bug...

@benjamingeer
Copy link
Author

Fixed more bugs by adding the test with a standoff timestamp tag (f70fe6c).

@tobiasschweizer
Copy link
Contributor

With which release should this be integrated?

@benjamingeer
Copy link
Author

With which release should this be integrated?

It's not going into today's release. It could go into next month's release. Let's talk about this process at tomorrow's meeting.

@tobiasschweizer
Copy link
Contributor

Ok! I would like to assign related PRs in knra-api-js-lib the same milestone tags. This should make it easy to keep track.

@benjamingeer benjamingeer added this to the 2020-02 milestone Jan 27, 2020
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

Can the SALSAH GUI display a time value? Are the value type constants correctly configured?

@benjamingeer
Copy link
Author

Can the SALSAH GUI display a time value? Are the value type constants correctly configured?

You mean the old GUI? I guess it can't. Which constants are those?

@tobiasschweizer
Copy link
Contributor

Which constants are those?

I think they are hard-coded in a JS file. They had to be changed to the Knora IRIs. Since the time value hasn't been supported yet by Knora, it is possible that the value type IRI has to be adapted. Maybe @loicjaouen knows this.

@benjamingeer
Copy link
Author

OK, I think I've got it working in SALSAH 1 in 68ab4d5. I didn't really know what I was doing, but I can create a resource with a time value, display it, and change the time value. Could you have a look?

@tobiasschweizer
Copy link
Contributor

Could you have a look?

I haven't run it in years. @loicjaouen Could you have a look?

@benjamingeer
Copy link
Author

@subotic I'm having trouble finding anyone who can review the changes I made in the SALSAH 1 JavaScript code for this PR. Could you help find a reviewer, or should we just merge it anyway?

@loicjaouen
Copy link
Collaborator

sorry, I missed the call, I look at it now

Copy link
Collaborator

@loicjaouen loicjaouen left a comment

Choose a reason for hiding this comment

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

Code looks fine and I tested the timestamp in the GUI, add, edit, store and search work: it behaves as it should

@benjamingeer
Copy link
Author

Thanks @loicjaouen !

@benjamingeer
Copy link
Author

@tobiasschweizer Is this OK to merge now?

@tobiasschweizer
Copy link
Contributor

I will look at it in a minute!

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

Looks good! When you merge this PR, please also merge dasch-swiss/dsp-js-lib#59. Thanks!

@benjamingeer
Copy link
Author

@tobiasschweizer Thank you!

@benjamingeer benjamingeer merged commit d925c85 into develop Feb 5, 2020
@benjamingeer benjamingeer deleted the wip/1397-time-value branch February 5, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler warnings Missing definition "TimeValue" in knora-base
6 participants