Skip to content

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Mar 29, 2024

Why these changes are being introduced:

The "access to files" values have been changed in the data mapping
layer. We need to make a corresponding change in a few partials
where the value of that field is a rendering condition.

Relevant ticket(s):

How this addresses that need:

This updates the values as needed in the access_button, record_geo, and
geo_data_info partials, as well as the access_type helper method.

Side effects of this change:

It was a bit tricky to wrangle and update all of these hardcoded values.
Though they are unlikely to change again, this is something that
we may want to consider refactoring.

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

To confirm the changes, check a full record for each of the three access types. The download buttons should contain the following text, based on the access type:

  • MIT authentication required: "Download geodata files fa-lock icon MIT authentication"
    * no authentication required: "Download geodata files"
    * unknown: check with owning institution: "View institution name 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-252-ac-lqel9i March 29, 2024 16:02 Inactive
@coveralls
Copy link

coveralls commented Mar 29, 2024

Pull Request Test Coverage Report for Build 8485137780

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 99.069%

Totals Coverage Status
Change from base Build 8473683589: 0.2%
Covered Lines: 532
Relevant Lines: 537

💛 - Coveralls

@matt-bernhardt matt-bernhardt self-assigned this Mar 29, 2024
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

The change here seems fine, and I see it reflected in the review app. I have a question, though, about some other references to the access_type helper - for example, whether the one here needs to have its value updated as well?

https://github.com/MITLibraries/timdex-ui/blob/main/app/views/record/_record_geo.html.erb#L45

I ask because when I look at record gismit:US_P1TRANTERM_2006 I'm seeing a "Download full metadata" button on the desktop screen, and I'm curious if that's still intended - although maybe that's going to be dealt with in a separate ticket.

At any rate, my question is whether there are other uses of this helper which also need to be updated.

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-252-ac-lqel9i March 29, 2024 19:39 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-252-ac-lqel9i March 29, 2024 19:44 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Mar 29, 2024

@matt-bernhardt Ah yeah, good catch. In the hasty process of trying to fixing this, I'd forgotten how many views that method helps.

To answer your question, that 'download metadata' button should appear for all MIT-provided records, including the one you shared. It should not, however, appear for the OGM records, since it would be the same URL as the access button link.

So, a couple more things to confirm in this most recent change:

  • For MIT records, the access button in a full record downloads the shapefiles.
  • For non-MIT records, the access button in a full record links to the record on the hosting repository.
  • The following access signal appears below the title, next to the date and content type (this can be confirmed either in the result or record view, as they both use the same partial):
    • For MIT-auth records, "MIT authentication required fa-lock"
    • For non-MIT records, "Not owned by MIT (owned by host institution)", with the parenthetical a link to the record

Thanks for reviewing this with greater care than I wrote it!

Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

This all looks good to me - thanks for noting the sorts of conditions that I should be looking for (which I believe I've identified in my poking at the review app).

:shipit:

@jazairi jazairi force-pushed the gdt-252-access-type-update branch from dbb45fc to 4a2f581 Compare March 29, 2024 20:09
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-252-ac-lqel9i March 29, 2024 20:09 Inactive
Why these changes are being introduced:

The "access to files" values have been changed in the data mapping
layer. We need to make a corresponding change in a few partials
where the value of that field is a rendering condition.

Relevant ticket(s):

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

How this addresses that need:

This updates the values as needed in the access_button, record_geo, and
geo_data_info partials, as well as the access_type helper method.

Side effects of this change:

It was a bit tricky to wrangle and update all of these hardcoded values.
Though they are unlikely to change again, this is something that
we may want to consider refactoring.
@jazairi jazairi force-pushed the gdt-252-access-type-update branch from 4a2f581 to c7f05d4 Compare March 29, 2024 20:13
@jazairi jazairi changed the title Update access type labels for access buttons Update access type values Mar 29, 2024
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-252-ac-lqel9i March 29, 2024 20:13 Inactive
@jazairi jazairi merged commit 84d9194 into main Mar 29, 2024
@jazairi jazairi deleted the gdt-252-access-type-update branch March 29, 2024 20:14
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