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

Graceful handling of partial SKOS XL data for concept labels #1356

Merged
merged 3 commits into from
Sep 12, 2022

Conversation

osma
Copy link
Member

@osma osma commented Sep 7, 2022

Reasons for creating this PR

This PR changes the way SKOS XL data for labels is handled. The old code expected that if a concept has some SKOS XL labels, it will have SKOS XL data for all labels. This isn't the case in current YSO where SKOS XL information is added only for a small number of labels. This PR makes the checks for SKOS XL data more careful. Result now looks like this (compare with issue #1346):

image

Link to relevant issue(s), if any

Description of the changes in this PR

  • Concept.hasXlLabel() and ConceptPropertyValueLiteral.hasXlLabel() methods changed to be more specific so that they only return true when there is non-trivial SKOS XL data available specifically for that concept (prefLabel) / literal value
  • some logic (filtering of trivial properties rdf:type and skosxl:literalForm) moved from Twig templates to PHP code
  • use dc: namespace for gettext lookups for properties in the dc11: namespace also for properties of SKOS XL Labels (this is already done for e.g. regular properties on concepts and it fixes issues displaying the correct labels for Dublin Core metadata)

Known problems or uncertainties in this PR

  • when displaying properties of SKOS XL labels with literal values (e.g. dc:source "Kansalliskirjasto"@fi in above screenshot), there is no mechanism for trying to find and select the value with the correct language tag; so if there were many similar values in different languages, only one of them would be shown
  • I noticed that if I hover over the "info" symbol many times, eventually the tooltip will become empty; this could be a bug in qtip2 but it should go a way if/when we replace it with CSS (see PR Replace qtip JS library by a pure CSS tooltip #1324)

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added the bug label Sep 7, 2022
@osma osma added this to the 2.16 milestone Sep 7, 2022
@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #1356 (a1b6e2c) into master (5e6da4b) will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1356      +/-   ##
============================================
+ Coverage     70.68%   71.15%   +0.47%     
- Complexity     1646     1649       +3     
============================================
  Files            32       32              
  Lines          3786     3786              
============================================
+ Hits           2676     2694      +18     
+ Misses         1110     1092      -18     
Impacted Files Coverage Δ
model/Concept.php 86.41% <100.00%> (+2.77%) ⬆️
model/ConceptPropertyValueLiteral.php 95.55% <100.00%> (-0.10%) ⬇️
model/LabelSkosXL.php 39.13% <100.00%> (+39.13%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@osma osma requested a review from joelit September 7, 2022 12:05
Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

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

The handling of concept.xlLabel.properties tooltip for the title row seems to be broken. Examples in the comments. OTOH, this could be merged as a fix to the original bug of concept preflabels not being shown at all.

@joelit
Copy link
Contributor

joelit commented Sep 7, 2022

I tested his with SKOS-XL data and it is indeed an improvement - the labels work now but the tooltips don't.

With concept data looking like this:

yso:p6095 a skos:Concept,
        yso-meta:Concept ;
    dct:created "1985-01-01"^^xsd:date ;
    skos:altLabel "Betula"@en,
        "birch"@en,
        "koivu"@fi,
        "björk"@sv ;
    skos:broader yso:p14339,
        yso:p3425 ;
    skos:closeMatch <http://id.loc.gov/authorities/subjects/sh85014281>,
        allars:Y16158,
        ysa:Y96273 ;
    skos:inScheme yso: ;
    skos:narrower yso:p16291,
        yso:p26048,
        yso:p6094 ;
    skos:prefLabel "birches"@en,
        "koivut"@fi,
        "soahki"@se,
        "björkar"@sv ;
    skos:related yso:p21624 ;
    skosxl:prefLabel <http://www.yso.fi/onto/yso-meta/p6095#se_1> .

<http://www.yso.fi/onto/yso-meta/p6095#se_1> a skosxl:Label ;
    dc:source "Lähde Giellagáldu"@fi,
        "Gáldu Giellagáldu"@se ;
    skos:inScheme yso: ;
    skosxl:literalForm "soahki"@se .

The result in Skosmos UI looks like this. The labels work and Skosmos picks the right source information from the skosxl:Label object. The tooltips work when they are displayed in the concept information (foreign label info in this case), but the tooltips won't work as part of the title information.

Screenshot from 2022-09-07 17-20-12
Seems to work fine here!

Screenshot from 2022-09-07 17-20-27
Oddly, not working here!

@osma
Copy link
Member Author

osma commented Sep 12, 2022

Thanks for the review and testing @joelit !

I think what you are seeing is a different bug, probably having more to do with qtip2 than the changes in this PR. I hope that replacing qtip2 with CSS in PR #1324 will help solve that issue.

It did work for me, at least briefly (on Firefox):

image

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 4 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma
Copy link
Member Author

osma commented Sep 12, 2022

I added more unit tests, now the diff coverage is 100% 📈
Ready for a final round of review.

@osma osma marked this pull request as ready for review September 12, 2022 11:12
@osma osma requested a review from joelit September 12, 2022 11:13
Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@osma osma merged commit 0640b5e into master Sep 12, 2022
@osma osma deleted the issue1346-partial-skos-xl-data branch September 12, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial SKOS XL label data breaks headline on concept page
2 participants