Merge branch 'master' into beta

* master: MBS-12870: Trap focus inside the Popover component (#2826) MBS-12863: Tooltips too far to the left (#2819) MBS-12888: Check ended when copying end date Actually look at stringy dates with isDateEmpty Autocomplete2: For "noop" actions, hide the menu instead MBS-12872: Autocomplete2: Don't focus the first item unless searching MBS-12885: Instrument disappears after adding another Sort attributes passed to fromDistinctAscArray MBS-12874: Don't split new relationships with one instrument or vocal
metabrainz · Feb 8, 2023 · fadb8a7 · fadb8a7
2 parents 8d2fc18 + 50a7a28
commit fadb8a7
Show file tree

Hide file tree

Showing 17 changed files with 386 additions and 42 deletions.
diff --git a/root/static/scripts/common/components/Autocomplete2.js b/root/static/scripts/common/components/Autocomplete2.js
@@ -551,11 +551,9 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>(
 
       case 'Enter':
       case 'Tab': {
-        if (isOpen) {
+        if (isOpen && highlightedItem) {
           event.preventDefault();
-          if (highlightedItem) {
-            selectItem(highlightedItem);
-          }
+          selectItem(highlightedItem);
         }
         break;
       }
@@ -822,6 +820,7 @@ const Autocomplete2 = (React.memo(<+T: EntityItemT>(
             ? 'visible'
             : 'hidden',
         }}
+        tabIndex="-1"
       >
         {disabled ? null : menuItemElements}
       </ul>

diff --git a/root/static/scripts/common/components/Autocomplete2/actions.js b/root/static/scripts/common/components/Autocomplete2/actions.js
@@ -20,10 +20,6 @@ export const HIGHLIGHT_PREVIOUS_ITEM = {
   type: 'highlight-previous-item',
 };
 
-export const NOOP = {
-  type: 'noop',
-};
-
 export const SHOW_MENU = {
   type: 'set-menu-visibility',
   value: true,

diff --git a/root/static/scripts/common/components/Autocomplete2/constants.js b/root/static/scripts/common/components/Autocomplete2/constants.js
@@ -10,7 +10,7 @@
 import {bracketedText} from '../../utility/bracketed.js';
 
 import {
-  NOOP,
+  HIDE_MENU,
   SEARCH_AGAIN,
   SHOW_MORE_RESULTS,
   TOGGLE_INDEXED_SEARCH,
@@ -71,19 +71,19 @@ export const MENU_ITEMS: {+[name: string]: ActionItemT<empty>, ...} = {
   },
   LOOKUP_ERROR: {
     type: 'action',
-    action: NOOP,
+    action: HIDE_MENU,
     id: 'lookup-error',
     name: N_l('An error occurred while looking up the MBID you entered.'),
   },
   LOOKUP_TYPE_ERROR: {
     type: 'action',
-    action: NOOP,
+    action: HIDE_MENU,
     id: 'lookup-type-error',
     name: N_l('The type of entity you pasted isn’t supported here.'),
   },
   NO_RESULTS: {
     type: 'action',
-    action: NOOP,
+    action: HIDE_MENU,
     id: 'no-results',
     name: () => bracketedText(l('No results')),
   },

diff --git a/root/static/scripts/common/components/Autocomplete2/reducer.js b/root/static/scripts/common/components/Autocomplete2/reducer.js
@@ -372,6 +372,7 @@ export function runReducer<+T: EntityItemT>(
   const wasOpen = state.isOpen;
   let updateItems = false;
   let updateStatusMessage = false;
+  let highlightFirstIndex = false;
 
   switch (action.type) {
     case 'change-entity-type': {
@@ -417,9 +418,6 @@ export function runReducer<+T: EntityItemT>(
       break;
     }
 
-    case 'noop':
-      break;
-
     case 'toggle-add-entity-dialog': {
       state.isAddEntityDialogOpen = action.isOpen;
       break;
@@ -477,7 +475,7 @@ export function runReducer<+T: EntityItemT>(
          * where "Show more" was clicked).
          */
       } else {
-        state.highlightedIndex = 0;
+        highlightFirstIndex = true;
       }
 
       state.results = newResults;
@@ -563,6 +561,7 @@ export function runReducer<+T: EntityItemT>(
         if (nonEmpty(newInputValue)) {
           // We'll display "(No results)" even if `results` is null.
           state.isOpen = true;
+          highlightFirstIndex = true;
         }
       } else {
         state.results = null;
@@ -595,9 +594,8 @@ export function runReducer<+T: EntityItemT>(
     state.statusMessage = generateStatusMessage(state);
   }
 
-  // Highlight the first item by default.
   const isOpen = state.isOpen;
-  if (isOpen && (!wasOpen || state.highlightedIndex < 0)) {
+  if (isOpen && highlightFirstIndex) {
     state.highlightedIndex = getFirstHighlightableIndex(state);
   } else if (wasOpen && !isOpen) {
     state.highlightedIndex = -1;
@@ -608,10 +606,6 @@ export default function reducer<+T: EntityItemT>(
   state: StateT<T>,
   action: ActionT<T>,
 ): StateT<T> {
-  if (action.type === 'noop') {
-    return state;
-  }
-
   const nextState = {...state};
   runReducer<T>(nextState, action);
   return nextState;

diff --git a/root/static/scripts/common/components/Autocomplete2/types.js b/root/static/scripts/common/components/Autocomplete2/types.js
@@ -69,7 +69,6 @@ export type ActionT<+T: EntityItemT> =
   | { +type: 'highlight-index', +index: number }
   | { +type: 'highlight-next-item' }
   | { +type: 'highlight-previous-item' }
-  | { +type: 'noop' }
   | { +type: 'reset-menu' }
   | { +type: 'select-item', +item: ItemT<T> }
   | { +type: 'set-menu-visibility', +value: boolean }

diff --git a/root/static/scripts/common/components/Popover.js b/root/static/scripts/common/components/Popover.js
@@ -76,7 +76,7 @@ const Popover = (props: PropsT): React.Portal => {
       dialogRef={dialogRef}
       onEscape={closeAndReturnFocus}
       siblings={<div data-popper-arrow />}
-      trapFocus={false}
+      trapFocus
     >
       {buildChildren(closeAndReturnFocus)}
     </Dialog>,

diff --git a/root/static/scripts/common/utility/isDateEmpty.js b/root/static/scripts/common/utility/isDateEmpty.js
@@ -7,15 +7,9 @@
  * later version: http://www.gnu.org/licenses/gpl-2.0.txt
  */
 
-declare type PartialDateStringsT = {
-  +day?: string,
-  +month?: string,
-  +year?: string,
-};
-
 export default function isDateEmpty(
   date: ?PartialDateT | ?PartialDateStringsT,
 ): boolean %checks {
   return (date == null) ||
-    (date.year == null && date.month == null && date.day == null);
+         (empty(date.year) && empty(date.month) && empty(date.day));
 }
diff --git a/root/static/scripts/edit/components/DateRangeFieldset.js b/root/static/scripts/edit/components/DateRangeFieldset.js
@@ -137,13 +137,21 @@ export function runReducer(
       const year = String(beginDateFields.year.value ?? '');
       const month = String(beginDateFields.month.value ?? '');
       const day = String(beginDateFields.day.value ?? '');
+      const newEndDate: PartialDateStringsT =
+        {day: day, month: month, year: year};
       runFormRowPartialDateReducer(
         subfields.end_date,
         {
-          date: {day: day, month: month, year: year},
+          date: newEndDate,
           type: 'set-date',
         },
       );
+      if (!isDateEmpty(newEndDate)) {
+        runReducer(
+          state,
+          {enabled: true, type: 'set-ended'},
+        );
+      }
       validateDatePeriod(state);
       applyAllPendingErrors(state);
       break;

diff --git a/root/static/scripts/relationship-editor/components/RelationshipEditor.js b/root/static/scripts/relationship-editor/components/RelationshipEditor.js
@@ -446,11 +446,22 @@ export function runReducer(
         const updates = [];
 
         if (oldRelationshipState != null) {
-          updates.push({
-            relationship: oldRelationshipState,
-            throwIfNotExists: false,
-            type: REMOVE_RELATIONSHIP,
-          });
+          /*
+           * The old relationship state must be removed first in a separate
+           * `updateRelationships` call, because its presence affects other
+           * functions that act on the current state, like
+           * `mergeRelationship`.
+           */
+          updateRelationships(
+            writableState,
+            [
+              {
+                relationship: oldRelationshipState,
+                throwIfNotExists: false,
+                type: REMOVE_RELATIONSHIP,
+              },
+            ],
+          );
         }
 
         updates.push(...getUpdatesForAcceptedRelationship(

diff --git a/root/static/scripts/relationship-editor/utility/splitRelationshipByAttributes.js b/root/static/scripts/relationship-editor/utility/splitRelationshipByAttributes.js
@@ -28,6 +28,7 @@ import type {
 
 import {cloneRelationshipState} from './cloneState.js';
 import {
+  areLinkAttributesEqual,
   compareLinkAttributeIds,
   compareLinkAttributeRootIds,
   compareLinkAttributes,
@@ -238,6 +239,21 @@ export default function splitRelationshipByAttributes(
     }
   }
 
+  /*
+   * If (1) this is a new relationship, (2) `newAttributesToSplit` contains a
+   * single attribute, and (3) it's identical to `newAttributes`, then
+   * there's nothing to split; we can just return the relationship as-is.
+   * (Triggered MBS-12874 in a roundabout way.)
+   */
+  if (
+    origRelationship == null &&
+    newAttributesToSplit != null &&
+    newAttributesToSplit.size === 1 &&
+    areLinkAttributesEqual(newAttributes, newAttributesToSplit)
+  ) {
+    newAttributesToSplit = null;
+  }
+
   for (const linkAttribute of tree.iterate(newAttributesToSplit)) {
     const newRelationship = cloneRelationshipState(relationship);
     newRelationship._lineage = [

diff --git a/root/static/scripts/tests/relationship-editor.js b/root/static/scripts/tests/relationship-editor.js
@@ -15,6 +15,7 @@ import {
 
 import {defaultContext} from '../../../context.mjs';
 import linkedEntities from '../common/linkedEntities.mjs';
+import {uniqueNegativeId} from '../common/utility/numbers.js';
 import {
   createInitialState,
   reducer,
@@ -271,7 +272,7 @@ test('merging duplicate relationships', function (t) {
 });
 
 test('splitRelationshipByAttributes', function (t) {
-  t.plan(14);
+  t.plan(16);
 
   const lyre = {
     type: {
@@ -371,7 +372,7 @@ test('splitRelationshipByAttributes', function (t) {
         // Add a new credit to the existing lyre attribute.
         {...lyre, credited_as: 'LYRE'},
         drums,
-      ]),
+      ].sort(compareLinkAttributeIds)),
       compareLinkAttributeIds,
       onConflictUseSecondValue,
     ),
@@ -595,7 +596,12 @@ test('splitRelationshipByAttributes', function (t) {
     ...existingRelationship4,
     _original: existingRelationship4,
     _status: REL_STATUS_EDIT,
-    attributes: tree.fromDistinctAscArray([drums, leadVocals]),
+    attributes: tree.fromDistinctAscArray(
+      [
+        drums,
+        leadVocals,
+      ].sort(compareLinkAttributeIds),
+    ),
   };
 
   splitRelationships =
@@ -626,6 +632,43 @@ test('splitRelationshipByAttributes', function (t) {
     ),
     'second relationship contains drums',
   );
+
+  /*
+   * This test attempts to split a newly-added relationship with a single
+   * vocal attribute. Since there's no need to split a single attribute,
+   * we should return the same relationship as-is (MBS-12874).
+   */
+
+  const newRelationship1: RelationshipStateT = {
+    _lineage: [],
+    _original: null,
+    _status: REL_STATUS_ADD,
+    attributes: tree.fromDistinctAscArray([leadVocals]),
+    begin_date: null,
+    editsPending: false,
+    end_date: null,
+    ended: false,
+    entity0: artist,
+    entity0_credit: '',
+    entity1: event,
+    entity1_credit: '',
+    id: uniqueNegativeId(),
+    linkOrder: 0,
+    linkTypeID: 798,
+  };
+  Object.freeze(newRelationship1);
+
+  splitRelationships =
+    splitRelationshipByAttributes(newRelationship1);
+
+  t.ok(
+    splitRelationships.length === 1,
+    'one relationships is returned',
+  );
+  t.ok(
+    splitRelationships[0] === newRelationship1,
+    'the same relationship is returned back',
+  );
 });
 
 function addRelationships(

diff --git a/root/static/styles/relationship-editor.less b/root/static/styles/relationship-editor.less
@@ -1,7 +1,15 @@
 #content.rel-editor #tracklist td {vertical-align: top;}
 #content.rel-editor #tracklist td.midcol {vertical-align: middle; font-size: @small-text;}
 #content.rel-editor #tracklist input[type="checkbox"] {vertical-align: middle;}
-#content.rel-editor #batch-tools {width: 100%; margin: 0.8em;}
+#content.rel-editor #batch-tools {
+    width: 100%;
+    margin: 0.8em;
+    /*
+     * Center the tooltips so that they're less likely to clip
+     * outside the viewport.
+     */
+    .tooltip-container { left: 50%; }
+}
 #content.rel-editor div.ars {padding: 0; margin: 0.5em 0; margin-left: 1.5em;}
 #content.rel-editor #release-rels {margin-top: 1em;}
 

diff --git a/root/static/styles/tooltip.less b/root/static/styles/tooltip.less
@@ -8,7 +8,7 @@
         visibility: hidden;
         position: absolute;
         top: -12px;
-        left: 50%;
+        left: 100%;
         z-index: 1000;
 
         .tooltip-triangle-mixin {

diff --git a/root/types/entity.js b/root/types/entity.js
@@ -96,6 +96,12 @@ declare type PartialDateT = {
   +year: number | null,
 };
 
+declare type PartialDateStringsT = {
+  +day?: string,
+  +month?: string,
+  +year?: string,
+};
+
 declare type TypeRoleT<T> = {
   +typeID: number | null,
   +typeName?: string,

diff --git a/t/selenium.mjs b/t/selenium.mjs
@@ -630,6 +630,8 @@ const seleniumTests = [
   {name: 'MBS-11730.json5', login: true},
   {name: 'MBS-11735.json5', login: true},
   {name: 'MBS-12859.json5', login: true},
+  {name: 'MBS-12874.json5', login: true},
+  {name: 'MBS-12885.json5', login: true},
   {name: 'Artist_Credit_Editor.json5', login: true},
   {name: 'Autocomplete2.json5'},
   {name: 'CAA.json5', login: true},