Update MITAardvark 'rights' field method to support access filter #114

jonavellecuerdo · 2024-01-24T16:33:06Z

Purpose and background context

The planned UI mockup for the updated TIMDEX UI includes a feature to filter records
based on "Access to files". This requires a static, aggregatable string value to be present
in a TIMDEX record that can be used by the TIMDEX API to retrieve relevant records
from OpenSearch.

How can a reviewer manually see the effects of these changes?

Review updated test_aardvark_get_rights_success: tests/sources/json/test_aardvark.py.

Run make test and confirm all tests are passing.

Includes new or updated dependencies?

NO

Changes expectations for external applications?

YES

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/GDT-138

Developer

All new ENV is documented in README
All new ENV has been added to staging and production environments
All related Jira tickets are linked in commit message(s)
Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

The commit message is clear and follows our guidelines (not just this PR message)
There are appropriate tests covering any new functionality
The provided documentation is sufficient for understanding any new functionality introduced
Any manual tests have been performed and verified
New dependencies are appropriate or there were no changes

ghukill

Looking good! Just left a comment about the logic (which looks good) and it's relationship to required fields, and gismit vs gisogm records. But as stated in the comment, likely out of scope for this PR.

transmogrifier/sources/json/aardvark.py

ehanson8

I like the approach, just some questions and suggestions about testing and fixtures

tests/fixtures/aardvark/aardvark_record_all_fields.jsonl

tests/sources/json/test_aardvark.py

ehanson8 · 2024-01-24T18:31:36Z

transmogrifier/sources/json/aardvark.py

+            kind_access_rights = "Access"
+            kind_access_to_files = "Access to files"


I get "Access to files" but I'm unclear about the difference here, what is "Access" referring to? And it looks like kind_access_rights is only used once so does it need to be a variable?

I am not sure of the intention of kind = "Access", but all it seems to do is retrieve the value from the Aardvark field dct_accessRights_s. For the MITAardvark transformer, the get_rights() field method essentially pulls from fields under the Aardvark Rights category.

@ehanson8 and @ghukill - What do you think of updating kind="Access" -> kind="Access rights"?

Maybe a better way of stating my question is: is there actually a difference between Access and Access to files? There may not be but if we have 2 different strings, the user may think there's a difference

I agree with @ehanson8 about "Access" vs "Access to files" being very similar and potentially confusing. But the language for the values, and the language for that kind both come directly from the UI mockup:

While maybe we could make the kind something like "Download Access", we'd need to confirm with EngX that the actual "kind" string could differ from the text on the UI. I imagine it could, but unsure.

OR, we could just continue with "Access to files" (which does kind of suggest downloading), and then iterate later if determined confusing.

As far as the string values go (e.g. "Not owned by MIT"), I'd imagine those are good as-is because they will render directly on the page via the aggregation.

I'll keep it as-is for now since this is a draft and update as needed later! Whatever we decide, I think it could (maybe) be a good thing if the names align in transmogrifier and the timdex/UI...

I'm OK with that as is given the draft status and I get it now seeing the UI, but to me Access to files seems like the only type of access users would care about. But this is better sorted out in inter-team discussions later!

Some context (this may no longer be relevant, I haven't checked to see what you all decided on in the code).

In general, field values can be manipulated between OpenSearch and TIMDEX API (or TIMDEX API and TIMDEX UI) when we have a good reason to do so, but when we find ourselves wanting to do that we should probably pause and ask why. If UX is suggesting a label in the UI is the best way to describe something from a user perspective, is there a reason an API user would not find the same language helpful? Or is there a reason we want to introduce potential future confusion between what is in OpenSearch and TIMDEX API?

So before forking our language across tiers we should reach out to UX and explain the situation and ask if they would consider language we feel is more specific or accurate. If there is a good reason to make a distinction between how we refer to data in TIMDEX Spec and TIMDEX API/TIMDEX UI, we can definitely support it... we just need to be sure that's what we want :)

I better see your point @ehanson8, that conceptually what's even the difference... And, agreed about inter-team discussing, which this slack thread might tease out.

AND, one of the open questions for GDT-138 is whether this will even be a Rights entry, or maybe a dedicated field, so might all be moot if it's a new field.

transmogrifier/sources/json/aardvark.py

ehanson8 · 2024-01-24T18:43:29Z

transmogrifier/sources/json/aardvark.py

+            if source == "gisogm":
+                rights.append(
+                    timdex.Rights(
+                        description="Not owned by MIT", kind=kind_access_to_files


Curious to get GIS staff feedback on phrasing, I don't know what is expected here (or if it only matters as a facet)

@ehanson8 Thank you for the really helpful suggestions! Can you clarify what you mean by this comment? 🤔

I think it is likely that we may have MITAardvark and OGMAardvark classes since we hardcoded a gismit string into MITAardvark.get_source_link() but we may find a better solution that avoids that

jonavellecuerdo · 2024-01-25T15:00:42Z

@ehanson8 @ghukill Let me know what you think of the latest changes!

The UI mockup indicates "MIT Authentication", but I propose "Required MIT authentication" so that all string values will be adjectives. 🤓
Acknowledging that the if-else conditions in MITAardvark.get_rights() field method may not be needed once we create an OGMAardvark transformer.
We will circle back on Rights.kind values and will check in with EngX (@JPrevost) on whether or not the values in the TIMDEX record can (or should) differ from the TIMDEX (Geoweb) UI.

JPrevost · 2024-01-25T15:25:09Z

@jonavellecuerdo I haven't read through the full discussion here, but is there a transform in dev1 S3 that has been created with this branch so I can pull in the data to better understand the ramifications?

jonavellecuerdo · 2024-01-26T16:38:08Z

@JPrevost Thank you for asking! Here is an example of a transform in dev1 S3 with the proposed changes: JSON file. Also, I'll be sending over some notes from the spike work investigating nested, filtered aggregations in OpenSearch (heads up -- yes we can 😄 ).

Why these changes are being introduced: * Enable filtering TIMDEX records based on "Access to files" by providing a static, aggregatable string value indicating the category for a given record. How this addresses that need: * Update MITAardvark.get_rights field method * Accept MITAardvark.source as an argument to determine ownership * Use "dct_accessRights_s" to determine access category Side effects of this change: * Need to determine whether OpenSearch can limit aggregation to a field that is filtered by a subfield (i.e., Rights.kind) and make necessary updates to timdex-ui. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-138

…s rights'

jonavellecuerdo · 2024-02-20T18:00:25Z

@ehanson8 @ghukill

Please see the latest commit. Here are the main takeaways:

Ultimately, we confirmed that the proposed solution of introducing a new instance of "Rights" to TimdexRecord.rights field to support "Access to file" filters is (a) technically achievable with openSearch and (b) it is reasonable to use this field in this manner, as exemplified by at least one other transformer.
During one of our sync-up meetings, @ghukill and I thought that a solution to address the comment on "Access" vs. "Access to files" could be to update the code such that:

timdex.Rights(description=<geoharvester.harvester.records.MITAardvark.dct_accessRights_s>, kind="Access rights")

I decided to update "Requires MIT authentication" -> "MIT authentication" to align the value with the proposed UI's display.

Let me know what you think!

ghukill

Code looks good, approved.

I imagine the TIMDEX data model questions will continue (and currently are for other fields), but this code looks good given the decision to store this information in TIMDEX.rights.

ehanson8

One suggested change

transmogrifier/sources/json/aardvark.py

…ords Why these changes are being introduced: * This change is required in order to perform aggregations on the 'description' property of the 'rights' field. More specifically, this work is to enable filtering based on "Access to files" and support the updates to the TIMDEX data model in the PR: MITLibraries/transmogrifier#114 How this addresses that need: * Update config to convert 'rights.description' to a multifield that indexes the value as a "keyword" in a similarly named subfield (i.e., 'description.keyword') Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-138

* Update MITAardvark 'rights' field method to support access filter Why these changes are being introduced: * Enable filtering TIMDEX records based on "Access to files" by providing a static, aggregatable string value indicating the category for a given record. How this addresses that need: * Update MITAardvark.get_rights field method * Accept MITAardvark.source as an argument to determine ownership * Use "dct_accessRights_s" to determine access category * Update Rights.kind for MITAardvark.dct_accessRights_s field to 'Access rights' Side effects of this change: * Need to determine whether OpenSearch can limit aggregation to a field that is filtered by a subfield (i.e., Rights.kind) and make necessary updates to timdex-ui. * timdex-index-manager (TIM) needs to update `rights.description` as a multifield with a keyword value to enable aggregation. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-138

jonavellecuerdo requested review from ghukill and ehanson8 January 24, 2024 16:33

ghukill reviewed Jan 24, 2024

View reviewed changes

transmogrifier/sources/json/aardvark.py Outdated Show resolved Hide resolved

ehanson8 reviewed Jan 24, 2024

View reviewed changes

jonavellecuerdo force-pushed the GDT-138-update-timdex-record-to-enable-access-filter branch 2 times, most recently from 8ec640f to ad4303a Compare January 25, 2024 14:50

jonavellecuerdo requested review from ghukill and ehanson8 January 25, 2024 15:05

jonavellecuerdo force-pushed the GDT-138-update-timdex-record-to-enable-access-filter branch from ad4303a to 0ec0931 Compare February 20, 2024 17:19

jonavellecuerdo added 2 commits February 20, 2024 12:37

Update Rights.kind for MITAardvark.dct_accessRights_s field to 'Acces…

8c54755

…s rights'

jonavellecuerdo force-pushed the GDT-138-update-timdex-record-to-enable-access-filter branch from bd820db to 8c54755 Compare February 20, 2024 17:39

jonavellecuerdo marked this pull request as ready for review February 20, 2024 18:00

ghukill approved these changes Feb 20, 2024

View reviewed changes

ehanson8 approved these changes Feb 20, 2024

View reviewed changes

transmogrifier/sources/json/aardvark.py Show resolved Hide resolved

jonavellecuerdo merged commit c75fd04 into main Feb 21, 2024
5 checks passed

jonavellecuerdo deleted the GDT-138-update-timdex-record-to-enable-access-filter branch February 21, 2024 16:35

jonavellecuerdo mentioned this pull request Feb 21, 2024

Update field mapping for 'rights.description' to index values as keyw… MITLibraries/timdex-index-manager#310

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MITAardvark 'rights' field method to support access filter #114

Update MITAardvark 'rights' field method to support access filter #114

jonavellecuerdo commented Jan 24, 2024 •

edited

Loading

ghukill left a comment

ehanson8 left a comment

ehanson8 Jan 24, 2024

jonavellecuerdo Jan 24, 2024

ehanson8 Jan 24, 2024

ghukill Jan 25, 2024 •

edited

Loading

jonavellecuerdo Jan 25, 2024

ehanson8 Jan 25, 2024

JPrevost Jan 25, 2024

ghukill Jan 25, 2024

ehanson8 Jan 24, 2024

jonavellecuerdo Jan 24, 2024

ehanson8 Jan 24, 2024

jonavellecuerdo commented Jan 25, 2024

JPrevost commented Jan 25, 2024

jonavellecuerdo commented Jan 26, 2024

jonavellecuerdo commented Feb 20, 2024

ghukill left a comment

ehanson8 left a comment

		kind_access_rights = "Access"
		kind_access_to_files = "Access to files"

Update MITAardvark 'rights' field method to support access filter #114

Update MITAardvark 'rights' field method to support access filter #114

Conversation

jonavellecuerdo commented Jan 24, 2024 • edited Loading

Purpose and background context

How can a reviewer manually see the effects of these changes?

Includes new or updated dependencies?

Changes expectations for external applications?

What are the relevant tickets?

Developer

Code Reviewer(s)

ghukill left a comment

Choose a reason for hiding this comment

ehanson8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghukill Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonavellecuerdo commented Jan 25, 2024

JPrevost commented Jan 25, 2024

jonavellecuerdo commented Jan 26, 2024

jonavellecuerdo commented Feb 20, 2024

ghukill left a comment

Choose a reason for hiding this comment

ehanson8 left a comment

Choose a reason for hiding this comment

jonavellecuerdo commented Jan 24, 2024 •

edited

Loading

ghukill Jan 25, 2024 •

edited

Loading