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

Order mappings on concept page first by source vocabulary, then by label #1601

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

osma
Copy link
Member

@osma osma commented Mar 5, 2024

Reasons for creating this PR

Cypress tests for the mappings on the concept page were failing, because the two mappings on the example page to "musicology" from Wikidata and "Musicology" from LCSH were not in the expected order. It turned out that the order of mappings depended on the labels only and wasn't stable in a case like this were the labels are the same (case insensitive).

This PR causes the mappings on the concept page (of each type) to be displayed in a consistent order: first by source vocabulary, then by label. This is implemented on the backend side, so that the data returned by the mappings REST API method is sorted. (An alternative approach would have been to do the sorting in the frontend only.)

Result can be seen e.g. on the YSO concept page for "musicology":

image

Note that the closeMatch mappings are ordered by vocabulary (LCSH < Wikidata) and likewise the exactMatch mappings are now ordered by vocabulary (Allärs < KOKO < YSA). Before this PR, the order could be different as it was based only on the labels (e.g. music research < musiikintutkimus < musikforskning) and could be arbitrary when labels are the same (musicology vs Musicology).

Link to relevant issue(s), if any

Description of the changes in this PR

  • add a getSortKey method to the classes ConceptPropertyValue, ConceptPropertyValueLiteral and ConceptMappingPropertyValue (for the two former, based on label only; for the mappings, based first on vocabulary name, then by label)
  • use the getSortKey method when sorting property values in ConceptProperty.sortValues()
  • add PHPUnit tests for the getSortKey methods
  • adjust the Cypress test for the concept page mappings so that it matches the new order of mapping values

Known problems or uncertainties in this PR

n/a

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 this to the 3.0 milestone Mar 5, 2024
@osma osma self-assigned this Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.58%. Comparing base (4921604) to head (ed30a50).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1601      +/-   ##
============================================
+ Coverage     70.54%   70.58%   +0.04%     
- Complexity     1644     1647       +3     
============================================
  Files            32       32              
  Lines          4315     4321       +6     
============================================
+ Hits           3044     3050       +6     
  Misses         1271     1271              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarqubecloud bot commented Mar 5, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@osma osma requested a review from joelit March 6, 2024 08:57
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 9e02d76 into main Mar 6, 2024
12 checks passed
@osma osma deleted the issue1484-concept-page-mappings-fix-order branch March 6, 2024 09:38
@osma osma modified the milestones: 3.x, 3.0 Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

2 participants