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

EDSC-3909: Fix showing direct collection -> variable associations #1687

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

eudoroolivares2016
Copy link
Contributor

@eudoroolivares2016 eudoroolivares2016 commented Oct 20, 2023

Overview

What is the feature?

There was a user requested fix to showing the variables associated to the collection in a project utilizing Harmony or OpenDap services. In EDSC-3817 a change was made to include only variables associated to the services of a collection. Instead this ticket updates this business logic to show the variables associated directly to the collection unless there exists variables associated to a service of the collection.

What is the Solution?

Tweak business logic in modules for building the access method component

What areas of the application does this impact?

Access methods

Testing

Add a collection to your project which implements opendap and or harmony services. Ensure that on the variables page (select Edit Variables on the Configure data customization options) we get a view of all of the variables associated to that collection if there exists no services on that collection which have variable associations.

Secondly ensure that for a collection which does contain variables associated to the services those variables are instead shown

This query from gaphql

query ($params: CollectionsInput) {
  collections(params: $params) {
    items {
      conceptId
      variables {
        count
        items {
          name
          conceptId
        }
      }
      services {
        items {
          variables {
            count
            items {
              name
              conceptId
            }
          }
        }
      }
    }
  }
}

along with the concept-id as a parameter can be used to validate that the metadata for the collection does/does not have either type of association.

Attachments

Using C1996881146-POCLOUD in the production envs
image

Using C1200277763-E2E_18_4 in the SIT env
image

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #1687 (8954f4f) into main (b9523c5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1687      +/-   ##
==========================================
+ Coverage   91.91%   91.93%   +0.01%     
==========================================
  Files         724      724              
  Lines       19339    19344       +5     
  Branches     4553     4540      -13     
==========================================
+ Hits        17776    17783       +7     
+ Misses       1427     1426       -1     
+ Partials      136      135       -1     
Files Coverage Δ
...ic/src/js/util/accessMethods/buildAccessMethods.js 86.95% <100.00%> (+1.01%) ⬆️
static/src/js/util/accessMethods/getVariables.js 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@@ -17,11 +17,13 @@ import { supportsVariableSubsetting } from './supportsVariableSubsetting'
export const buildAccessMethods = (collectionMetadata, isOpenSearch) => {
const {
granules = {},
services = {}
services = {},
variables: collAssociatedVars = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't paying by the character here, spell out your variable names ;)

@eudoroolivares2016 eudoroolivares2016 marked this pull request as ready for review October 20, 2023 01:36
macrouch
macrouch previously approved these changes Oct 20, 2023
} = serviceItem

if (serviceAssociatedVariables.count > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the service has no variables associated with it, does serviceAssociatedVariables come back as null or { count: 0, items: null }?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first test where you moved the variables to the collection level, it still needs to return something for variables because the graphql query is requesting that field. So updating that test to look correct should answer my question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes back as { count: 0, items: null } in the graphql response

@macrouch macrouch self-requested a review October 20, 2023 03:00
@macrouch macrouch dismissed their stale review October 20, 2023 03:01

Needs changes

Switching to key off of `items` count is more confusing
@eudoroolivares2016 eudoroolivares2016 merged commit 584ba39 into main Oct 20, 2023
9 checks passed
@eudoroolivares2016 eudoroolivares2016 deleted the EDSC-3909 branch October 20, 2023 20:16
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.

3 participants