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

DAS-1901: Adding publishVariableDrafts field to publish generated variable drafts. #79

Merged
merged 11 commits into from
Nov 22, 2023

Conversation

william-valencia
Copy link
Contributor

@william-valencia william-valencia commented Nov 2, 2023

Overview

What is the feature?

DAS-1901: Adding the publishVariableDrafts field to the collection query to use earthdata-varinfo to generate and publish variable drafts.

What is the Solution?

Added publishVariableDrafts field to the collection query to use earthdata-varinfo to generate and publish variable drafts. Also obtained the rest of the variable info from CMR since earthdata-varinfo only returns the concept ids.

What areas of the application does this impact?

the collection Query will have an additional field for publishVariableDrafts

Testing

Reproduction steps

  • Need to test in UAT or PROD
  • C1245618475-EEDTEST or similar
  1. Use Apollo server to query collection publishVariableDrafts field. See README.

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

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

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (38851d5) 100.00% compared to head (6c447a5) 100.00%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #79   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines         1516      1544   +28     
  Branches       208       209    +1     
=========================================
+ Hits          1516      1544   +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

publishVariableDrafts needs to be a mutation, not a return field within a query.

Other work being done this PI also makes the name publishVariableDrafts too similar to the publishDraft mutation, so maybe something like publishGeneratedVariables would be more clear about what exactly is expected of the mutation

src/types/variable.graphql Outdated Show resolved Hide resolved

const { dataSources } = context

const publish = true
Copy link
Member

Choose a reason for hiding this comment

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

I would just pass true below, future developers can look at the arguments of the method to understand what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its passed as a key/value below, so If I remove this const, Id have to pass it as "publish: true" below. Would that be preferred?

src/datasources/collectionVariableDrafts.js Outdated Show resolved Hide resolved
@abbottry abbottry self-requested a review November 21, 2023 19:29

// Pull out the variable concept ids from the source to use as parameters later
const conceptIds = {
params: { conceptId: Object.values(results).map((object) => Object.values(object)).flat() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we split this up a bit or double check these all calls need to be made here?

{"conceptId": "V0002-TEST"}
]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline

"Version": "1.8.2"
}
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline

@william-valencia william-valencia merged commit 06b7ba2 into main Nov 22, 2023
6 checks passed
@william-valencia william-valencia deleted the DAS-1901-2 branch November 22, 2023 17:36
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