Support ordered rdf list for concept property#1863
Conversation
|
osma
left a comment
There was a problem hiding this comment.
Thank you @jdfranel , this is an excellent PR. You have covered various aspects really well and the contribution fits in to the existing code base. Kudos!
Although we don't currently have the need to support rdf:List values in concept properties at our institution, I see that you have this need and that this could be useful for other Skosmos users.
A couple of minor comments:
-
You have made the truncation limit configurable (default to 1000, but set to 10 in the test configuration so that it can be tested). Are you thinking of this limit as a backstop to prevent damage in the case of extremely long or malformed (perhaps looping) lists? Or is this something that you would routinely expect to be set at a low value such as 10 or 50? I'm asking this because this is a bit similar to the way lists of values are currently truncated at 15 items (see #1825 and/or open the page http://localhost:9090/altlabel/en/page/c1 after running
tests/init_containers.sh). In those cases a link is shown that can be clicked by the user to reveal the rest of the items. They already exist in the HTML DOM, they are just hidden by default. I'm wondering whether these two truncation features should be better aligned. But if your use case is to prevent damage in case of excessively long lists rather than improve the UX, then there is no need to align these features. -
Normally we require Cypress tests for UI layer features. Do you think you could provide these? They could be a part of
tests/cypress/template/concept.cy.jsand just verify that the page shows a) non-truncated b) truncated lists. Basically the same as the PHPUnit tests, but from the HTML DOM perspective i.e. checking the rendering but not necessarily all the underlying logic as the PHPUnit test already verifies that. -
Regarding the SonarCloud issue you had: yes, it's a bit unfortunate that SonarCloud flags HTTP URIs as suspicious. You found a good solution to that with the comment! We normally use the SonarCloud UI to mark these as "not a problem" but it's a bit of manual work every time new URIs are introduced and it requires access to the SonarCloud project so it can't be done by contributors like you.
|
|
Hi @osma , Alignment with existing truncation features: I've updated the visual truncation script to handle RDF lists in the same way as other multi-valued properties. Lists are now shown with a "show all" link when they exceed the default display limit of 15 items, maintaining consistency with the existing UX patterns. Regarding rdfListItemsLimit: This configuration parameter serves as a backstop to prevent performance issues with excessively long or potentially malformed (looping) RDF lists. Since RDF list processing requires recursive SPARQL queries to traverse the list structure, very long lists could significantly impact server response time. The default value of 1000 should handle most legitimate use cases while protecting against more extreme cases. I considered using a default of 0 (unlimited), but opted for the conservative approach to avoid unexpected performance degradation on production systems. This is a different concern from the UI truncation - it's about query performance rather than user experience. Cypress tests: I've added Cypress tests in tests/cypress/template/rdf-list.cy.js that verify:
Let me know if there is other changes you want me to make. |
osma
left a comment
There was a problem hiding this comment.
Thanks, this is excellent. I spotted a little typo ("Trucated") but I will fix it myself, then merge this.
Reasons for creating this PR
I am working on setting up a vocabulary that needs to allow for an ordered list of related concepts but the skosmos UI does only show the blank node of the first element in the list instead of showing the as it does with the unordered list.
Link to relevant issue(s), if any
Closes #1864
Description of the changes in this PR
To support this ordered list I did the following changes:
skosmos:rdfListItemsLimitto limit the maximum number of element in the list (default to 1000Also, while testing my changes I had tu edit the tests so they use the env
SKOSMOS_SPARQL_ENDPOINTinstead of the static'http://localhost:9030/skosmos/sparql'in tests/ModelTest.php and tests/VocabularyTest.phpKnown problems or uncertainties in this PR
Checklist
.sr-onlyclass, color contrast)