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

Initial draft for listing new and modified concepts for a vocabulary in the REST API #982

Merged
merged 31 commits into from
Jun 15, 2020

Conversation

joelit
Copy link
Contributor

@joelit joelit commented May 6, 2020

Output can be seen e.g.
curl -X GET --header 'Accept: application/json' http://localhost/Skosmos/rest/v1/yso/modifiedConcepts?lang=en'

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #982 into master will increase coverage by 6.08%.
The diff coverage is 58.82%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #982      +/-   ##
============================================
+ Coverage     59.12%   65.20%   +6.08%     
- Complexity     1548     1829     +281     
============================================
  Files            32       32              
  Lines          4335     5280     +945     
============================================
+ Hits           2563     3443     +880     
- Misses         1772     1837      +65     
Impacted Files Coverage Δ Complexity Δ
controller/RestController.php 11.45% <34.78%> (+0.89%) 189.00 <17.00> (+17.00)
controller/WebController.php 14.98% <64.28%> (+8.47%) 83.00 <3.00> (+6.00)
model/sparql/GenericSparql.php 74.53% <91.66%> (+6.12%) 438.00 <3.00> (+121.00)
model/Vocabulary.php 86.80% <100.00%> (-0.26%) 112.00 <1.00> (-1.00)
model/ConceptProperty.php 96.29% <0.00%> (+0.46%) 28.00% <0.00%> (+11.00%)
model/Concept.php 82.08% <0.00%> (+2.28%) 253.00% <0.00%> (+66.00%)
model/VocabularyConfig.php 95.62% <0.00%> (+2.55%) 142.00% <0.00%> (+61.00%)
controller/Honeypot.php 46.15% <0.00%> (+7.69%) 11.00% <0.00%> (ø%)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80fad12...a907434. Read the comment docs.

@osma osma added this to the 2.6 milestone May 7, 2020
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

This is how the JSON output of modifiedConcepts currently looks:

kuva

Some observations:

  • JSON-LD context is missing for most fields/properties
  • The results are grouped by month, as they are shown in the UI. I think that is unnecessary in a REST API. It would be better to just return a flat list.
  • The dates are very complex objects with several fields. It would be better to just return ISO 8601 datetime strings such as 2020-05-04T02:44:12+00:00, which can then be assigned the xsd:dateTime data type in the JSON-LD context

controller/RestController.php Outdated Show resolved Hide resolved
rest.php Outdated Show resolved Hide resolved
@joelit joelit force-pushed the issue600-rest-changed-concepts branch 2 times, most recently from 3ade80f to 855ef57 Compare May 8, 2020 15:22
@joelit joelit force-pushed the issue600-rest-changed-concepts branch from 855ef57 to ce77bc6 Compare May 12, 2020 06:52
@joelit joelit marked this pull request as ready for review May 12, 2020 08:22
@joelit joelit requested a review from osma May 12, 2020 08:24
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Some rather minor remarks

controller/RestController.php Outdated Show resolved Hide resolved
controller/RestController.php Outdated Show resolved Hide resolved
controller/WebController.php Outdated Show resolved Hide resolved
@osma
Copy link
Member

osma commented May 12, 2020

Other comments on the PR:

  • Swagger spec still needs to be done
  • I wonder if the methods should support setting limit and offset, like some other REST methods (e.g. search, index/X)

@joelit joelit linked an issue May 12, 2020 that may be closed by this pull request
@osma
Copy link
Member

osma commented May 12, 2020

Scrutinizer complains about the getChangeList method docstring:

  • @param string $lang UI language for the dates
    There is no parameter named $lang. Was it maybe removed?

Method signature:

public function getChangeList($prop, $clang, $offset, $limit)

@osma osma modified the milestones: 2.6, Next Tasks May 12, 2020
@osma
Copy link
Member

osma commented May 12, 2020

Postponing to the next release, will release 2.6 without this PR since there are still things to improve

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Still a few things to improve/fix, but looking pretty good now

controller/WebController.php Outdated Show resolved Hide resolved
model/Vocabulary.php Outdated Show resolved Hide resolved
swagger.json Outdated Show resolved Hide resolved
swagger.json Outdated Show resolved Hide resolved
model/sparql/GenericSparql.php Outdated Show resolved Hide resolved
tests/RestControllerTest.php Show resolved Hide resolved
@joelit joelit requested a review from kouralex June 12, 2020 17:11
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

A couple of minor phpdoc bugs, otherwise ready for merging

model/sparql/GenericSparql.php Outdated Show resolved Hide resolved
model/sparql/GenericSparql.php Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joelit joelit merged commit c76c510 into master Jun 15, 2020
@joelit joelit deleted the issue600-rest-changed-concepts branch June 15, 2020 07:51
@miguelvaara miguelvaara modified the milestones: Next Tasks, 2.7 Jun 24, 2020
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.

Add REST API method for new concepts list
4 participants