MMT-3409: publishDraft mutation for Collection/Service/Tool drafts, deleteTool mutation #73

macrouch · 2023-10-19T17:29:22Z

Overview

What is the feature?

Adds publishDraft mutation to publish Collection/Service/Tool drafts.
Adds deleteTool mutation to delete Tools

What areas of the application does this impact?

Publishing drafts, deleting tools

Testing

Publishing a draft (needs a valid draft in CMR to publish)

    mutation PublishDraft(
      $draftConceptId: String!
      $nativeId: String!
      $ummVersion: String!
    ) {
      publishDraft(
        draftConceptId: $draftConceptId
        nativeId: $nativeId
        ummVersion: $ummVersion
      ) {
        conceptId
        revisionId
        warnings
        existingErrors
      }
    }

variables:

    {
      "draftConceptId": "TD1000000001-EXAMPLE",
      "nativeId": "tool-1",
      "ummVersion": "1.2.0"
    }

Deleting a Tool

    mutation DeleteTool(
      $providerId: String!
      $nativeId: String!
    ) {
      deleteTool(
        providerId: $providerId
        nativeId: $nativeId
      ) {
        conceptId
        revisionId
      }
    }

variables:

    {
      "providerId": "EXAMPLE",
      "nativeId": "tool-1"
    }

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

abbottry · 2023-10-20T15:36:43Z

src/cmr/concepts/__tests__/draft.test.js

+
+          expect(draft.ingestPath).toEqual('providers/EDSC/bad-drafts')
+          expect(draft.publishPath).toEqual('publish/CD100000-EDSC')
+          expect(draft.metadataSpecification).toEqual({


Seems like this should throw an error instead of return bad data... Is the idea here that we send this to CMR and let them report the issue?

Its really unreachable code. The conceptType in that switch statement is based off a graphql enum type, but I didn't want to write a ton of if statements so I wrote a switch. eslint expects a default case so this test handles the default. I can change it to throw an error, but it is still something we wouldn't see

Seeing all the tests I broke when I changed it to throw an error. When we call publishDraft it still uses this class, but conceptType is unimportant, so I just passed in Draft, which hits the default case in the switch. But the metadataSpecification isn't used when publishing a draft

abbottry · 2023-10-20T15:37:08Z

src/cmr/concepts/draft.js


 import { pickIgnoringCase } from '../../utils/pickIgnoringCase'
 import Concept from './concept'
+import { mergeParams } from '../../utils/mergeParams'


Clean up import order :)

abbottry · 2023-10-20T15:38:28Z

src/cmr/concepts/draft.js

    const {
-      providerId,
-      ummVersion
+      draftConceptId = '',


This feels like a required field, what is the purpose of defaulting it to an empty string versus throwing an error?

I think the empty string came from an earlier attempt to work this where I was performing some action on the string, not needed anymore

abbottry · 2023-10-20T15:39:29Z

src/cmr/concepts/draft.js

+    let ummType
+    let ummName
+    let ummVersion
+    switch (conceptType) {


Can't the sub classes just set/return these values instead of having a switch statement in the parent?

There are no sub classes

abbottry · 2023-10-20T15:41:19Z

src/cmr/concepts/draft.js

+  publish(data, requestedKeys, providedHeaders) {
+    const { ummVersion } = data
+
+    // eslint-disable-next-line no-param-reassign


I see this 6 times in the repo -- wouldn't it be better to create a new const and remove the eslint-disable?

abbottry · 2023-10-20T15:43:36Z

src/datasources/__tests__/tool.test.js

+
+    process.env = { ...OLD_ENV }
+
+    process.env.cmrRootUrl = 'http://example.com'


I've been trying to be more specific in the URLs in tests to indicate what service its calling, example-cmr in this case -- it makes the testing a bit more specific and easier to read.

abbottry · 2023-10-20T15:45:43Z

src/types/tool.graphql

+
+type ToolMutationResponse {
+  "The unique concept id assigned to the tool."
+  conceptId: String


This is required in other types, should it be required here?

abbottry · 2023-10-20T15:45:48Z

src/types/tool.graphql

+  "The unique concept id assigned to the tool."
+  conceptId: String
+  "The revision of the tool."
+  revisionId: String


This is required in other types, should it be required here?

codecov · 2023-10-24T12:51:16Z

Codecov Report

Merging #73 (b435637) into main (03a1819) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #73   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines         1446      1503   +57     
  Branches       202       208    +6     
=========================================
+ Hits          1446      1503   +57

Files	Coverage Δ
src/cmr/concepts/collectionDraftProposal.js	`100.00% <100.00%> (ø)`
src/cmr/concepts/concept.js	`100.00% <100.00%> (ø)`
src/cmr/concepts/draft.js	`100.00% <100.00%> (ø)`
src/cmr/concepts/draftConcept.js	`100.00% <100.00%> (ø)`
src/cmr/concepts/tool.js	`100.00% <100.00%> (ø)`
src/datasources/draft.js	`100.00% <100.00%> (ø)`
src/datasources/tool.js	`100.00% <100.00%> (ø)`
src/resolvers/collection.js	`100.00% <100.00%> (ø)`
src/resolvers/draft.js	`100.00% <100.00%> (ø)`
src/resolvers/tool.js	`100.00% <100.00%> (ø)`
... and 2 more

macrouch requested review from trevorlang, htranho, william-valencia, tle1989, dmistry1 and mandyparson October 19, 2023 17:53

cgokey approved these changes Oct 19, 2023

View reviewed changes

dmistry1 approved these changes Oct 20, 2023

View reviewed changes

abbottry reviewed Oct 20, 2023

View reviewed changes

macrouch force-pushed the MMT-3408 branch from 80a9dea to 33b3e90 Compare October 23, 2023 17:33

Base automatically changed from MMT-3408 to main October 23, 2023 22:35

macrouch force-pushed the MMT-3409 branch 2 times, most recently from 1ab3ea5 to c233749 Compare October 24, 2023 12:48

macrouch force-pushed the MMT-3409 branch from c416927 to f8537e1 Compare October 24, 2023 13:18

macrouch added 3 commits October 24, 2023 15:14

MMT-3409: Publish Collection/Service/Tool drafts

81674d3

MMT-3409: Adds deleteTool mutation

cc0b26c

MMT-3409: Refactor code

b435637

macrouch force-pushed the MMT-3409 branch from f8537e1 to b435637 Compare October 24, 2023 19:14

mandyparson approved these changes Oct 25, 2023

View reviewed changes

macrouch merged commit 8cbba28 into main Oct 25, 2023
6 checks passed

macrouch deleted the MMT-3409 branch October 25, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MMT-3409: publishDraft mutation for Collection/Service/Tool drafts, deleteTool mutation #73

MMT-3409: publishDraft mutation for Collection/Service/Tool drafts, deleteTool mutation #73

macrouch commented Oct 19, 2023

abbottry Oct 20, 2023

macrouch Oct 24, 2023

macrouch Oct 24, 2023

abbottry Oct 20, 2023

abbottry Oct 20, 2023

macrouch Oct 24, 2023

abbottry Oct 20, 2023

macrouch Oct 24, 2023

abbottry Oct 20, 2023

abbottry Oct 20, 2023

abbottry Oct 20, 2023

abbottry Oct 20, 2023

codecov bot commented Oct 24, 2023 •

edited

Loading


		process.env = { ...OLD_ENV }

		process.env.cmrRootUrl = 'http://example.com'

MMT-3409: publishDraft mutation for Collection/Service/Tool drafts, deleteTool mutation #73

MMT-3409: publishDraft mutation for Collection/Service/Tool drafts, deleteTool mutation #73

Conversation

macrouch commented Oct 19, 2023

Overview

What is the feature?

What areas of the application does this impact?

Testing

Checklist

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

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

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

codecov bot commented Oct 24, 2023 •

edited

Loading