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

CMR-9088: Add draft support for UMM-V to CMR graphql #51

Merged
merged 10 commits into from
May 5, 2023
Merged

Conversation

dmistry1
Copy link
Contributor

@dmistry1 dmistry1 commented May 2, 2023

Overview

Added UMM-V draft support.

What is the feature?

Added UMM-V draft support
Added all the field for UMM-V that are currently in MMT
Added test cases for

  • datasource/variableDraft
  • resolver/variableDraft

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #51 (ee6a909) into MMT-3263 (439102c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           MMT-3263       #51   +/-   ##
==========================================
  Coverage    100.00%   100.00%           
==========================================
  Files            57        59    +2     
  Lines          1215      1227   +12     
  Branches        155       155           
==========================================
+ Hits           1215      1227   +12     
Impacted Files Coverage Δ
src/resolvers/index.js 100.00% <ø> (ø)
src/datasources/variableDraft.js 100.00% <100.00%> (ø)
src/resolvers/variableDraft.js 100.00% <100.00%> (ø)

Copy link
Contributor

@macrouch macrouch left a comment

Choose a reason for hiding this comment

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

What version of UMM-V is MMT using? We should probably make sure there aren't any problems in the fields returned from cmr-graphql if there is a version mismatch. cmr-graphql is on UMM-V 1.7.

And I didn't think about that for the UMM-T PR, but cmr-graphql is on UMM-T 1.1. Those values can be found in serverless.yml, line 28


const contextValue = buildContextValue()

describe('Collection', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't testing a Collection

"associationDetails": "meta.association-details",
"conceptId": "meta.concept-id",
"dataType": "umm.DataType",
"definition": "umm.Definition",
"dimensions": "umm.Dimensions",
"fillValue": "umm.FillValue",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be plural

"The expanded or long name related to the variable Name."
longName: String
"The measurement information of a variable."
"The LonRange consists of an index range for longitude."
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't seem to match the field measurementIdentifiers

"associationDetails": "meta.association-details",
"conceptId": "meta.concept-id",
"dataType": "umm.DataType",
"definition": "umm.Definition",
"dimensions": "umm.Dimensions",
"fillValue": "umm.FillValue",
"latRange": "umm.LatRange",
Copy link
Contributor

Choose a reason for hiding this comment

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

latRange is not defined in variable.graphql. I also don't see it in the UMM-V model

"longName": "umm.LongName",
"lonRange": "umm.LonRange",
Copy link
Contributor

Choose a reason for hiding this comment

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

lonRange is not defined in variable.graphql. I also don't see it in the UMM-V model

"measurementIdentifiers": "umm.MeasurementIdentifiers",
"name": "umm.Name",
"nativeId": "meta.native-id",
"offset": "umm.Offset",
"relatedUrls": "umm.RelatedURLs",
"samplingIdentifiers": "umm.SamplingIdentifiers",
"sets": "umm.Sets",
Copy link
Contributor

Choose a reason for hiding this comment

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

A few of these newly added mappings are not in alphabetical order

Copy link
Contributor

Choose a reason for hiding this comment

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

indexRanges is defined in variable.graphql, but it not listed in the mapping. I think maybe it should be here but latRanges and lonRanges shouldn't?

"The scale is the numerical factor by which all values in the stored data field are multiplied in order to obtain the original values. May be used together with Offset. An example of a scale factor is '0.002'."
scale: Float
"The set information of a variable. The variable is grouped within a set. The set is defined by the name, type, size and index. For example, Name: 'Data_Fields', Type: 'General', Size: '15', Index: '7' for the case of the variable named 'LST_Day_1km'."
sets: JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure these fields are sorted alphabetically

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments from variable.graphql apply to this file. (I'd get that file fixed and copy/paste into the VariableDraft type, then remove what shouldn't be there)

}

input VariableDraftInput {
id: Int!
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this parameter isn't commented on the other Draft types, but it should be commented on all of them. Please add a comment describing the id field

Copy link
Contributor Author

Choose a reason for hiding this comment

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


const contextValue = buildContextValue()

describe('Variable', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable Draft

indexRanges: JSON
"The expanded or long name related to the variable Name."
longName: String
"The LonRange consists of an index range for longitude."
Copy link
Contributor

Choose a reason for hiding this comment

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

"The measurement information of a variable."

added "draft" for the describe name.
@dmistry1 dmistry1 merged commit 5e64d84 into MMT-3263 May 5, 2023
@macrouch macrouch deleted the CMR-9088 branch June 23, 2023 15:45
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.

None yet

3 participants