Issue #634 : Retrieve and display domains of ConceptScheme at the top of the tree hierarchy #660

tfrancart · 2017-11-17T14:26:44Z

Coming back with a PR for #634 .
I have made this cleaner.
I am resubmitting the PR, hoping it can be useful and integrated in the code.
This answers 2 client use-cases I have (UNESCO and another).

Could you please let me know how can this proposal can progress to be integrated in the codebase ?

Squashed commit of the following: commit 06f5f7a Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Mon Oct 30 12:03:56 2017 +0100 _ commit adf4cb8 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Mon Oct 30 11:40:25 2017 +0100 add a new key "tops" which contains the array of top concepts commit 7dfa174 Merge: 189e074 e4c7e61 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Fri Oct 20 09:32:23 2017 +0200 Merge branch 'skosmosWithDomain' of https://github.com/tfrancart/Skosmos into skosmosWithDomain commit 189e074 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Fri Oct 20 09:20:03 2017 +0200 Revert "adding timeline of concept versions" This reverts commit c24a834. commit 533b276 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Thu Oct 19 17:48:09 2017 +0200 Revert "Merge branch 'skosmosWithDomain' of https://github.com/tfrancart/Skosmos into skosmosWithDomain" This reverts commit 3f5e3a1, reversing changes made to c24a834. commit 4b1e306 Author: Thomas Francart <thomas.francart@sparna.fr> Date: Thu Oct 19 09:40:02 2017 +0200 Fixed typo commit 26947ea Author: Thomas Francart <thomas.francart@sparna.fr> Date: Thu Oct 19 09:23:25 2017 +0200 Cleaning, remove logs, rename variables and files in english commit e4c7e61 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Thu Oct 19 17:48:09 2017 +0200 Revert "Merge branch 'skosmosWithDomain' of https://github.com/tfrancart/Skosmos into skosmosWithDomain" This reverts commit 3f5e3a1, reversing changes made to c24a834. commit 3f5e3a1 Merge: c24a834 d36fc2d Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Thu Oct 19 16:28:07 2017 +0200 Merge branch 'skosmosWithDomain' of https://github.com/tfrancart/Skosmos into skosmosWithDomain commit c24a834 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Thu Oct 19 16:27:27 2017 +0200 adding timeline of concept versions commit d36fc2d Author: Thomas Francart <thomas.francart@sparna.fr> Date: Thu Oct 19 09:40:02 2017 +0200 Fixed typo commit 595415c Author: Thomas Francart <thomas.francart@sparna.fr> Date: Thu Oct 19 09:23:25 2017 +0200 Cleaning, remove logs, rename variables and files in english commit 0c62bb0 Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Thu Oct 12 12:35:49 2017 +0200 access to the hierarchy of a concept select in the list while taking into account the case where there is a domain or not and the case where there is one or more top concept commit e6a4b6d Author: CLARVIE <clarvie-edouard.yetikoua@sparna.fr> Date: Wed Oct 11 11:53:13 2017 +0200 adding domain in the hierarchy

domain nodes before non-domain nodes

https://github.com/tfrancart/Skosmos.git into skosmosWithDomains-single Conflicts: controller/RestController.php resource/js/hierarchy.js

true

https://github.com/tfrancart/Skosmos.git into skosmosWithDomains-single Conflicts: controller/RestController.php

Merge NatLibFi/master

osma

This looks promising, but is a bit hard to review because of the stylistic changes and reformatting.
Also I would suggest that you clean up the commit history by rebasing and squashing commits.
Please read this for some general guidance on making good PRs: https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests

osma · 2017-11-17T14:31:59Z

controller/RestController.php

@@ -200,6 +200,7 @@ public function vocabularyInformation($request)
                'skos' => 'http://www.w3.org/2004/02/skos/core#',
                'onki' => 'http://schema.onki.fi/onki#',
                'dct' => 'http://purl.org/dc/terms/',
+                'dcterms' =>'http://purl.org/dc/terms/',


Why do you need to define this prefix? dct is defined on the line above, you can just use that.

osma · 2017-11-17T14:32:10Z

controller/RestController.php

@@ -208,6 +209,7 @@ public function vocabularyInformation($request)
                'defaultLanguage' => 'onki:defaultLanguage',
                'languages' => 'onki:language',
                'label' => 'rdfs:label',
+                'subject' => 'dcterms:subject',


osma · 2017-11-17T14:32:18Z

controller/RestController.php

@@ -254,6 +256,7 @@ public function vocabularyStatistics($request)
                'void' => 'http://rdfs.org/ns/void#',
                'onki' => 'http://schema.onki.fi/onki#',
                'uri' => '@id',
+                'dcterms' =>'http://purl.org/dc/terms/',


osma · 2017-11-17T14:32:23Z

controller/RestController.php

@@ -549,6 +552,7 @@ private function returnDataResults($results, $format) {
                'dct' => 'http://purl.org/dc/terms/',
                'dc11' => 'http://purl.org/dc/elements/1.1/',
                'uri' => '@id',
+                'dcterms' =>'http://purl.org/dc/terms/',


osma · 2017-11-17T14:34:17Z

model/GlobalConfig.php

@@ -1,243 +1,243 @@
-<?php


Please don't make style fixes in unrelated code as part of a feature PR. It's practically impossible to see whether you've changed anything in this file, making it difficult to review. I suggest you revert this part and make it a separate PR if you like.

osma · 2017-11-17T14:41:13Z

resource/js/hierarchy.js

@@ -128,76 +128,126 @@ function createConceptObject(conceptUri, conceptData) {
 * @param {Object} parentData 
 */
 function attachTopConceptsToSchemes(schemes, currentNode, parentData) {


The changes in this file look more invasive than they are, because you've reformatted the code when editing it. Consider making it more like the original code so it's easier to see what has changed. However, I understand that this may be difficult to do afterwards.

osma · 2017-11-17T14:41:55Z

resource/js/hierarchy.js

+        	if(theSchemeLabel != '') { 
+	        	if((theScheme.subject) != null && (theScheme.subject.uri===theDomain.uri)) {
+	        		domainObject.children.push(
+	        				{


The indents look rather excessive here.

osma · 2017-11-17T14:42:48Z

resource/js/hierarchy.js

+    	}  else {
+    		return naturalCompare(this.get_text(a).toLowerCase(), this.get_text(b).toLowerCase());
+    	}
+    	// return naturalCompare(this.get_text(a).toLowerCase(), this.get_text(b).toLowerCase());


Please don't leave old code as comments. We have versioning for that.

osma · 2017-11-17T14:43:16Z

tests/GenericSparqlTest.php

@@ -350,6 +350,49 @@ public function testQueryConceptSchemes()
      $this->assertEquals('Test conceptscheme', $label['label']);
    }
  }
+


It's excellent that you've written tests for your new functionality!

osma · 2017-11-17T14:43:40Z

tests/GenericSparqlTest.php

@@ -738,7 +781,7 @@ public function testQueryParentList()
    );
    $props = array (
      'uri' => 'http://www.skosmos.skos/test/ta1',
-      'top' => 'http://www.skosmos.skos/test/conceptscheme',


Please don't drop the definition of top, it is still needed for backwards compatibility.

osma · 2017-11-17T14:50:29Z

Also, please rebase on top of current master. There are conflicts so this cannot be merged as-is.
Travis CI build failed, please take a look at that. I suspect the GlobalConfig changes might be the reason but not sure.

Fixed test to assert tops (plural) instead of 'opt' (singular) Deleted wrong test

…art/Skosmos into skosmosWithDomains-single

tfrancart · 2017-11-22T06:59:16Z

sigh -
You're probably right, but "rebasing" or "squashing" or modifying files without adding yet-another commit to the commit history is just something I don't know how to do properly.
I will close this PR, manually create another branch from master, re-do the modifications in a cleaner way in the new code, break it down in smaller chunks as you suggest, and open a new PR.

osma · 2017-11-22T07:19:07Z

Thanks for your patience!
Squashing is optional, it will just make the commit history cleaner by merging multiple successive commits into one. It's very easy to do (git rebase -i) and it's literally the opposite of "adding yet-another commit" :)
But the important part is not whether the history looks clean (it can be fixed later if necessary), it's that the PR applies cleanly (no conflicts), passes Travis checks and is easy enough to review. Contributing small bits at a time helps all these goals.

tfrancart and others added 17 commits October 31, 2017 10:00

Merge remote-tracking branch 'upstream/master'

5c28e7e

clean

7b7d55e

Beautifying code on PHP side

6bb6ef7

Beautifying hierarchy.js

293a433

Modified hierarchy.js to : 1/ handle multiple 'tops' value 2/ sort

cdc7dd3

domain nodes before non-domain nodes

Properly set the 'top' key from the 'tops' key in RestController

997482f

merge

60d313a

Merge branch 'skosmosWithDomains-single' of

7508813

https://github.com/tfrancart/Skosmos.git into skosmosWithDomains-single Conflicts: controller/RestController.php resource/js/hierarchy.js

Added subject as a key in @context of JSON-LD response

b7fb35b

Added 'tops' key on the top concepts if vocabulary has showHierarchy to

0337e03

true

Merge branch 'skosmosWithDomains-single' of

c8708b0

https://github.com/tfrancart/Skosmos.git into skosmosWithDomains-single Conflicts: controller/RestController.php

Added unit test on queryConceptSchemes to test subject retrieval

a5d625b

Fixed test to assert tops (plural) instead of 'opt' (singular)

63d80d4

Deleted wrong test

4caf82b

Merge pull request #4 from NatLibFi/master

0f33af3

Merge NatLibFi/master

Merge branch 'skosmos-canope' into skosmosWithDomains-single

96ef242

osma requested changes Nov 17, 2017

View reviewed changes

tfrancart added 2 commits November 21, 2017 17:19

Updated tests

38050b3

Fixed test to assert tops (plural) instead of 'opt' (singular) Deleted wrong test

Merge branch 'skosmosWithDomains-single' of https://github.com/tfranc…

109b9ea

…art/Skosmos into skosmosWithDomains-single

tfrancart closed this Nov 22, 2017

tfrancart mentioned this pull request Jan 5, 2021

Issue 634 v2 #1113

Merged

charlycou mentioned this pull request Aug 16, 2021

Concepts belonging to different conceptScheme are displayed side by side in hieirarchy while conceptScheme are used as top-level node #1185

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #634 : Retrieve and display domains of ConceptScheme at the top of the tree hierarchy #660

Issue #634 : Retrieve and display domains of ConceptScheme at the top of the tree hierarchy #660

tfrancart commented Nov 17, 2017

osma left a comment

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma Nov 17, 2017

osma commented Nov 17, 2017

tfrancart commented Nov 22, 2017

osma commented Nov 22, 2017

Issue #634 : Retrieve and display domains of ConceptScheme at the top of the tree hierarchy #660

Issue #634 : Retrieve and display domains of ConceptScheme at the top of the tree hierarchy #660

Conversation

tfrancart commented Nov 17, 2017

osma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osma commented Nov 17, 2017

tfrancart commented Nov 22, 2017

osma commented Nov 22, 2017