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
Collaborator

@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.

- Update OntologyV2R2RSpec.
- Update RDF4J.
- Fix code that searches for decimal values.
@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Jan 27, 2020

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 27, 2020

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
Collaborator Author

@benjamingeer benjamingeer commented Jan 27, 2020

Found the bug...

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 27, 2020

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

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Jan 27, 2020

With which release should this be integrated?

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 27, 2020

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

@tobiasschweizer tobiasschweizer commented Jan 27, 2020

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

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 28, 2020

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

@tobiasschweizer tobiasschweizer commented Jan 28, 2020

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
Collaborator Author

@benjamingeer benjamingeer commented Jan 28, 2020

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

@tobiasschweizer tobiasschweizer commented Jan 28, 2020

Could you have a look?

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

@benjamingeer benjamingeer requested a review from loicjaouen Jan 28, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 4, 2020

@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
Contributor

@loicjaouen loicjaouen commented Feb 4, 2020

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

Copy link
Contributor

@loicjaouen loicjaouen left a comment

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
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

Thanks @loicjaouen !

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

@tobiasschweizer Is this OK to merge now?

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Feb 5, 2020

I will look at it in a minute!

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Feb 5, 2020

@tobiasschweizer Thank you!

@benjamingeer benjamingeer merged commit d925c85 into develop Feb 5, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Unit Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
@benjamingeer benjamingeer deleted the wip/1397-time-value branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

6 participants