#2257 Unable to add a bond to a functional group by bond tool (#2494)

* \#2257 - Refactor fromBondAddition and update usage

* #2257 - Fix bond tool to add a bond on functional group

* #2257 - Update tests for `fromBondAddition`

* #2257 - Fix findItem to skip clicked functional group

* #2257 - Fix Bond Tool to link FG when starting on nothing

* #2257 - Fix Bond Tool linking Salts&Solvents

* #2257 - Fix FG to link multiple atoms

* #2257 - Add types or comments

* #2257 - Fix ci

packages/ketcher-core/__tests__/application/editor/actions/bond.test.ts

@@ -1,25 +1,23 @@
 import * as utils from 'application/editor/actions/utils'
 
 import { restruct, singleBond } from './data'
-
 import { fromBondAddition } from 'application/editor/actions'
 
-const [action, begin, end] = fromBondAddition(
-  restruct as any,
-  singleBond as any,
-  1,
-  undefined
-)
+const reStruct = Object.assign({}, restruct) as any
+reStruct.molecule.sgroups = []
+const [action, begin, end] = fromBondAddition(reStruct, singleBond, 1, {
+  label: 'C'
+})
 
 describe('Bond Addition', () => {
-  it('function atomForNewBond was called when end undefined', () => {
+  test('function `atomForNewBond` will be called if `endAtomPos` is `undefined`', () => {
     const spy = jest.spyOn(utils, 'atomForNewBond')
-    fromBondAddition(restruct as any, singleBond as any, 3, undefined)
+    fromBondAddition(reStruct, singleBond, 3, { label: 'C' })
     expect(spy).toHaveBeenCalled()
   })
-  it('function atomGetAttr was called', () => {
+  test('function `atomGetAttr` will be called', () => {
     const spy = jest.spyOn(utils, 'atomGetAttr')
-    fromBondAddition(restruct as any, singleBond as any, 5, 1)
+    fromBondAddition(reStruct, singleBond, 5, 1)
     expect(spy).toHaveBeenCalled()
   })
   it('should contain operation CalcImplicitH', () => {
@@ -34,10 +32,10 @@ describe('Bond Addition', () => {
     )
     expect(addFragment).toBeDefined()
   })
-  it('bond begin should be defined', () => {
+  test('bond begin should be defined', () => {
     expect(begin).toBeDefined()
   })
-  it('bond end should be defined', () => {
+  test('bond end should be defined', () => {
     expect(end).toBeDefined()
   })
 })

packages/ketcher-core/src/application/editor/actions/atom.ts

@@ -226,7 +226,7 @@ export function mergeFragmentsIfNeeded(action, restruct, srcId, dstId) {
   const frid = atomGetAttr(restruct, srcId, 'fragment')
   const frid2 = atomGetAttr(restruct, dstId, 'fragment')
   if (frid2 === frid || typeof frid !== 'number' || typeof frid2 !== 'number') {
-    return
+    return frid
   }
 
   const struct = restruct.molecule
@@ -249,6 +249,8 @@ export function mergeFragmentsIfNeeded(action, restruct, srcId, dstId) {
   mergeSgroups(action, restruct, fridAtoms, dstId)
   action.addOp(new FragmentDelete(frid2).perform(restruct))
   action.mergeWith(moveAtomsAction)
+
+  return frid
 }
 
 export function mergeSgroups(action, restruct, srcAtoms, dstAtom) {

packages/ketcher-core/src/application/editor/actions/bond.ts

@@ -20,7 +20,10 @@ import {
   Neighbor,
   StereoLabel,
   Struct,
-  Vec2
+  Vec2,
+  AtomAttributes,
+  BondAttributes,
+  FunctionalGroup
 } from 'domain/entities'
 import {
   AtomAdd,
@@ -46,77 +49,160 @@ import { StereoValidator } from 'domain/helpers'
 import utils from '../shared/utils'
 
 export function fromBondAddition(
-  restruct: ReStruct,
-  bond: any,
-  begin: any,
-  end: any,
-  pos?: Vec2,
-  pos2?: Vec2
+  reStruct: ReStruct,
+  bond: Partial<BondAttributes>,
+  begin: number | AtomAttributes,
+  end: number | AtomAttributes,
+  beginAtomPos?: Vec2,
+  endAtomPos?: Vec2
 ): [Action, number, number, number] {
-  // eslint-disable-line
-  if (end === undefined) {
-    const atom = atomForNewBond(restruct, begin, bond)
-    end = atom.atom
-    pos = atom.pos
-  }
   const action = new Action()
-  const struct = restruct.molecule
-  let mergeFragments = false
+  const struct = reStruct.molecule
+
+  const mouseDownNothingAndUpNothing = (
+    beginAtomAttr: AtomAttributes,
+    endAtomAttr: AtomAttributes
+  ) => {
+    const newFragmentId = (
+      action.addOp(new FragmentAdd().perform(reStruct)) as FragmentAdd
+    ).frid
+
+    const newBeginAtomId: number = (
+      action.addOp(
+        new AtomAdd(
+          { ...beginAtomAttr, fragment: newFragmentId },
+          beginAtomPos
+        ).perform(reStruct)
+      ) as AtomAdd
+    ).data.aid
+
+    const newEndAtomId: number = (
+      action.addOp(
+        new AtomAdd(
+          { ...endAtomAttr, fragment: newFragmentId },
+          endAtomPos
+        ).perform(reStruct)
+      ) as AtomAdd
+    ).data.aid
+
+    return [newBeginAtomId, newEndAtomId] as const
+  }
 
-  let frid = null
+  const mouseDownNothingAndUpAtom = (
+    beginAtomAttr: AtomAttributes,
+    endAtomId: number
+  ) => {
+    const fragmentId = atomGetAttr(reStruct, endAtomId, 'fragment')
+
+    const newBeginAtomId: number = (
+      action.addOp(
+        new AtomAdd(
+          { ...beginAtomAttr, fragment: fragmentId },
+          beginAtomPos
+        ).perform(reStruct)
+      ) as AtomAdd
+    ).data.aid
+
+    const endAtom = struct.atoms.get(endAtomId)
+    if (
+      endAtom &&
+      !FunctionalGroup.isAtomInContractedFunctionalGroup(
+        endAtom,
+        struct.sgroups,
+        struct.functionalGroups,
+        false
+      )
+    ) {
+      mergeSgroups(action, reStruct, [newBeginAtomId], endAtomId)
+    }
+    return [newBeginAtomId, endAtomId] as const
+  }
 
-  if (!(typeof begin === 'number')) {
-    if (typeof end === 'number') frid = atomGetAttr(restruct, end, 'fragment')
-  } else {
-    frid = atomGetAttr(restruct, begin, 'fragment')
-    if (typeof end === 'number') mergeFragments = true
+  const mouseDownAtomAndUpNothing = (
+    beginAtomId: number,
+    endAtomAttr: AtomAttributes
+  ) => {
+    const fragmentId = atomGetAttr(reStruct, beginAtomId, 'fragment')
+
+    const newEndAtomId: number = (
+      action.addOp(
+        new AtomAdd(
+          {
+            ...endAtomAttr,
+            fragment: fragmentId
+          },
+          endAtomPos ?? atomForNewBond(reStruct, begin, bond).pos
+        ).perform(reStruct)
+      ) as AtomAdd
+    ).data.aid
+
+    const beginAtom = struct.atoms.get(beginAtomId)
+    if (
+      beginAtom &&
+      !FunctionalGroup.isAtomInContractedFunctionalGroup(
+        beginAtom,
+        struct.sgroups,
+        struct.functionalGroups,
+        false
+      )
+    ) {
+      mergeSgroups(action, reStruct, [newEndAtomId], beginAtomId)
+    }
+
+    return [beginAtomId, newEndAtomId] as const
   }
 
-  if (frid == null) {
-    frid = (action.addOp(new FragmentAdd().perform(restruct)) as FragmentAdd)
-      .frid
+  let beginAtomId: number, endAtomId: number
+
+  const startsOnAtom = typeof begin === 'number'
+  const endsOnAtom = typeof end === 'number'
+
+  if (!startsOnAtom && !endsOnAtom) {
+    ;[beginAtomId, endAtomId] = mouseDownNothingAndUpNothing(begin, end)
+  } else if (!startsOnAtom && endsOnAtom) {
+    ;[beginAtomId, endAtomId] = mouseDownNothingAndUpAtom(begin, end)
+  } else if (startsOnAtom && !endsOnAtom) {
+    ;[beginAtomId, endAtomId] = mouseDownAtomAndUpNothing(begin, end)
+  } else {
+    ;[beginAtomId, endAtomId] = [begin as number, end as number]
   }
 
-  if (!(typeof begin === 'number')) {
-    begin.fragment = frid
-    begin = (action.addOp(new AtomAdd(begin, pos).perform(restruct)) as AtomAdd)
-      .data.aid
-    if (typeof end === 'number') mergeSgroups(action, restruct, [begin], end)
-    pos = pos2
-  } else if (atomGetAttr(restruct, begin, 'label') === '*') {
-    action.addOp(new AtomAttr(begin, 'label', 'C').perform(restruct))
+  if (atomGetAttr(reStruct, beginAtomId, 'label') === '*') {
+    action.addOp(new AtomAttr(beginAtomId, 'label', 'C').perform(reStruct))
   }
 
-  if (!(typeof end === 'number')) {
-    end.fragment = frid
-    // TODO: <op>.data.aid here is a hack, need a better way to access the id of a created atom
-    end = (action.addOp(new AtomAdd(end, pos).perform(restruct)) as AtomAdd)
-      .data.aid
-    if (typeof begin === 'number') mergeSgroups(action, restruct, [end], begin)
-  } else if (atomGetAttr(restruct, end, 'label') === '*') {
-    action.addOp(new AtomAttr(end, 'label', 'C').perform(restruct))
+  if (atomGetAttr(reStruct, endAtomId, 'label') === '*') {
+    action.addOp(new AtomAttr(endAtomId, 'label', 'C').perform(reStruct))
   }
 
-  const bid = (
-    action.addOp(new BondAdd(begin, end, bond).perform(restruct)) as BondAdd
+  const newBondId = (
+    action.addOp(
+      new BondAdd(beginAtomId, endAtomId, bond).perform(reStruct)
+    ) as BondAdd
   ).data.bid
-
-  const bnd = struct.bonds.get(bid)
-
-  if (bnd) {
-    action.addOp(new CalcImplicitH([bnd.begin, bnd.end]).perform(restruct))
-    action.mergeWith(fromBondStereoUpdate(restruct, bnd))
+  const newBond = struct.bonds.get(newBondId)
+  if (newBond) {
+    action.addOp(
+      new CalcImplicitH([newBond.begin, newBond.end]).perform(reStruct)
+    )
+    action.mergeWith(fromBondStereoUpdate(reStruct, newBond))
   }
 
   action.operations.reverse()
 
-  if (mergeFragments) mergeFragmentsIfNeeded(action, restruct, begin, end)
-
-  if (struct.frags.get(frid || 0)?.stereoAtoms && !bond.stereo) {
-    action.addOp(new FragmentStereoFlag(frid || 0).perform(restruct))
+  const mergedFragmentId = mergeFragmentsIfNeeded(
+    action,
+    reStruct,
+    beginAtomId,
+    endAtomId
+  )
+  if (struct.frags.get(mergedFragmentId || 0)?.stereoAtoms && !bond.stereo) {
+    action.addOp(
+      new FragmentStereoFlag(mergedFragmentId || 0).perform(reStruct)
+    )
   }
 
-  return [action, begin, end, bid]
+  return [action, beginAtomId, endAtomId, newBondId]
 }
 
 export function fromBondsAttrs(

packages/ketcher-core/src/application/editor/actions/chain.ts

@@ -53,7 +53,14 @@ export function fromChain(restruct, p0, v, nSect, atomId) {
   for (let i = 0; i < nSect; i++) {
     const pos = new Vec2(dx * (i + 1), i & 1 ? 0 : dy).rotate(v).add(p0)
 
-    const ret = fromBondAddition(restruct, {}, id0, {}, pos)
+    const ret = fromBondAddition(
+      restruct,
+      {},
+      id0,
+      { label: 'C' },
+      undefined,
+      pos
+    )
     action = ret[0].mergeWith(action)
     id0 = ret[2]
     chainItems.bonds.push(ret[3])

packages/ketcher-core/src/application/editor/actions/template.ts

@@ -55,6 +55,7 @@ function extraBondAction(restruct, aid, angle) {
       { type: 1 },
       aid,
       middleAtom.atom,
+      undefined,
       middleAtom.pos.get_xy0()
     )
     action = actionRes[0]

packages/ketcher-core/src/domain/entities/sgroup.ts

@@ -650,6 +650,18 @@ export class SGroup {
     })
     return !expanded && isSGroup
   }
+
+  /**
+   * @returns `undefined`: if it's salt or solvent
+   */
+  static getAttachmentAtomIdBySGroupId(sGroupId: number, struct: Struct) {
+    const functionalGroup = struct.functionalGroups.get(sGroupId)
+    if (!SGroup.isSaltOrSolvent(functionalGroup?.name || '')) {
+      const sGroup = struct.sgroups.get(sGroupId)
+      return sGroup?.getAttAtomId(struct)
+    }
+    return undefined
+  }
 }
 
 function descriptorIntersects(sgroups: [], topLeftPoint: Vec2): boolean {

packages/ketcher-react/src/script/editor/tool/atom.ts

@@ -178,7 +178,7 @@ class AtomTool {
       this.#bondProps,
       dragCtx.item.id,
       Object.assign({}, this.atomProps),
-      newAtomPos,
+      undefined,
       newAtomPos
     )[0]
     this.editor.update(dragCtx.action, true)