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

Reorganize GeoData full record based on usability test results #185

Merged
merged 1 commit into from
May 3, 2024

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented May 2, 2024

Why these changes are being introduced:

Now that we've done usability testing, we have a better sense of what fields in the 'More information' section of the full record may be useful.

Relevant ticket(s):

How this addresses that need:

This moves each relevant field out of the 'more_info_geo' partial and into the 'record_geo' partial. The display of each field has been cleaned up a bit (e.g., removing subfield keys), and the fields are now in p tags or ul tags with their own headings.

Since place names are duplicated as subjects, the locations field has been repurposed to show only geoshapes.

Some record helper logic (and corresponding tests) that are no longer needed have also been removed.

Side effects of this change:

I'm not sure it's useful to display this metadata, particularly when we provide an affordance to download the source metadata and/or link to the source record. At some point, I'd like to have a broader conversation about whether we should include full records at all in TIMDEX UI. For now, I'm comfortable implementing this as requested.

Developer

Accessibility
  • ANDI or WAVE has been run in accordance to our guide.
  • This PR contains no changes to the view layer.
  • New issues flagged by ANDI or WAVE have been resolved.
  • New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
  • No new accessibility issues have been flagged.
New ENV
  • All new ENV is documented in README.
  • All new ENV has been added to Heroku Pipeline, Staging and Prod.
  • ENV has not changed.
Approval beyond code review
  • UXWS/stakeholder approval has been confirmed.
  • UXWS/stakeholder review will be completed retroactively.
  • UXWS/stakeholder review is not needed.
Additional context needed to review

I haven't been able to find a record that includes all possible fields, but here's one that includes nearly all of them (missing 'alternate titles', which can be found in this record).

Code Reviewer

Code
  • I have confirmed that the code works as intended.
  • Any CodeClimate issues have been fixed or confirmed as
    added technical debt.
Documentation
  • The commit message is clear and follows our guidelines
    (not just this pull request message).
  • The documentation has been updated or is unnecessary.
  • New dependencies are appropriate or there were no changes.
Testing
  • There are appropriate tests covering any new functionality.
  • No additional test coverage is required.

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 15:02 Inactive
@coveralls
Copy link

coveralls commented May 2, 2024

Pull Request Test Coverage Report for Build 8939903883

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 98.625%

Files with Coverage Reduction New Missed Lines %
app/helpers/record_helper.rb 1 97.47%
Totals Coverage Status
Change from base Build 8924804527: -0.2%
Covered Lines: 574
Relevant Lines: 582

💛 - Coveralls

@jazairi jazairi force-pushed the gdt-283-reorganize-full-record branch from 350348d to 2f971f0 Compare May 2, 2024 16:03
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 16:03 Inactive
@jazairi jazairi force-pushed the gdt-283-reorganize-full-record branch from 2f971f0 to b34ba62 Compare May 2, 2024 16:45
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 16:45 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 17:04 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 17:06 Inactive
@JPrevost JPrevost self-assigned this May 2, 2024
@@ -118,6 +118,13 @@ def source_metadata_link(links)
links.select { |link| link['kind'] == 'Download' && link['text'] == 'Source Metadata' }.first['url']
end

# Geopoint is deprecated in the API, but that seems to be where all geospatial data is mapped. Including both for now.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem good. Are we sure this isn't just because TIMDEX UI is requesting the wrong field? TIMDEX API maps the data to both places but maybe we are requesting it from the wrong one in our query?

Copy link
Member

Choose a reason for hiding this comment

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

{
  search(searchterm: "rail", index: "geo") {
    hits
    records {
      title
      locations {
        geoshape
        geopoint
      }
    }
  }
}

Returns data in both fields (with geopoints being deprecated). I suspect what you are seeing is a TIMDEX UI oddity unless you have examples where it is working differently than I am seeing/understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah! You're right. 🤦 I thought I'd cross-checked this in the API, but it's duplicated in both fields as expected.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it's this that needs updating:
https://github.com/MITLibraries/timdex-ui/blob/main/app/models/timdex_record.rb#L63

I suspect we missed the "recordID" method updating when we added geo searches as we likely were focused only on searching and not full record views with that work.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to roll that into this PR or do a separate one for that and land it before this? I'm good with either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to roll it into this PR, but I'm having trouble getting the tests to not error with the regenerated cassettes. Stay tuned...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have just hit a run of bad luck. The test suite started passing after I ran it a handful of times. I'm glad that erratic behavior seems to mostly be limited to our locals...

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 2, 2024 21:04 Inactive
@jazairi jazairi requested a review from JPrevost May 2, 2024 21:07
@@ -0,0 +1,5 @@
require 'test_helper'
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to add some tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha. Relic of trying to get the suite to pass, which I'm not surprised made it into a late-afternoon push. (Hypothesis was that isolating the geo record tests in their own file would help; it didn't.)

Why these changes are being introduced:

Now that we've done usability testing, we have a better sense
of what fields in the 'More information' section of the full
record may be useful.

Relevant ticket(s):

* [GDT-283](https://mitlibraries.atlassian.net/browse/GDT-283)

How this addresses that need:

This moves each relevant field out of the 'more_info_geo' partial
and into the 'record_geo' partial. The display of each field has
been cleaned up a bit (e.g., removing subfield keys), and the
fields are now in `p` tags or `ul` tags with their own headings.

Since place names are duplicated as subjects, the locations field
has been repurposed to show only geoshapes. This also required
replacing the deprecated `geopoint` with `geoshape` in the
TimdexRecord query string, and regenerating the record controller
cassettes accordingly.

Some record helper logic (and corresponding tests) that are no
longer needed have also been removed.

Side effects of this change:

I'm not sure it's useful to display this metadata, particularly
when we provide an affordance to download the source metadata
and/or link to the source record. At some point, I'd like to have
a broader conversation about whether we should include full records
at all in TIMDEX UI. For now, I'm comfortable implementing this
as requested.
@jazairi jazairi force-pushed the gdt-283-reorganize-full-record branch from 2731740 to cfddc2d Compare May 3, 2024 13:38
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-283-re-uyis1j May 3, 2024 13:38 Inactive
@jazairi jazairi merged commit 924da70 into main May 3, 2024
5 checks passed
@jazairi jazairi deleted the gdt-283-reorganize-full-record branch May 3, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants