Merge pull request #6920 from jimchamp/6883/feature/adjust-mr-flow

Open merge UIs to `librarians`

conf/openlibrary.yml

@@ -65,7 +65,7 @@ features:
     lists: enabled
     merge-authors:
         filter: usergroup
-        usergroup: /usergroup/super-librarians
+        usergroup: /usergroup/librarians
     merge-editions: admin
     publishers: enabled
     recentchanges_v2: enabled

openlibrary/components/MergeUI.vue

@@ -1,14 +1,14 @@
 <template>
   <div id="app">
-    <MergeTable :olids="olids" :show_diffs="show_diffs" ref="mergeTable"/>
+    <MergeTable :olids="olids" :show_diffs="show_diffs" :primary="primary" ref="mergeTable"/>
     <div class="action-bar">
-        <div class="comment-input" v-if="mrid">
+        <div class="comment-input">
             <label for="comment">Comment: </label>
             <input name="comment" v-model="comment" type="text">
         </div>
         <div class="btn-group">
-            <button class="merge-btn" @click="doMerge" :disabled="mergeStatus != 'Do Merge'">{{mergeStatus}}</button>
-            <button class="reject-btn" v-if="mrid" @click="rejectMerge">Reject Merge</button>
+            <button class="merge-btn" @click="doMerge" :disabled="isDisabled">{{mergeStatus}}</button>
+            <button class="reject-btn" v-if="showRejectButton" @click="rejectMerge">Reject Merge</button>
         </div>
         <div id="diffs-toggle">
         <label>
@@ -24,6 +24,9 @@
 import MergeTable from './MergeUI/MergeTable.vue'
 import { do_merge, update_merge_request, createMergeRequest, DEFAULT_EDITION_LIMIT } from './MergeUI/utils.js';
 
+const DO_MERGE = 'Do Merge'
+const REQUEST_MERGE = 'Request Merge'
+
 export default {
     name: 'app',
     components: {
@@ -34,6 +37,14 @@ export default {
             type: [Number, String],
             required: false,
             default: ''
+        },
+        primary: {
+            type: String,
+            required: false
+        },
+        canmerge: {
+            type: String,
+            required: true
         }
     },
     data() {
@@ -48,13 +59,26 @@ export default {
     computed: {
         olids() {
             return this.url.searchParams.get('records', '').split(',')
+        },
+
+        isSuperLibrarian() {
+            return this.canmerge === 'true'
+        },
+
+        isDisabled() {
+            return this.mergeStatus !== DO_MERGE && this.mergeStatus !== REQUEST_MERGE
+        },
+
+        showRejectButton() {
+            return this.mrid && this.isSuperLibrarian
         }
     },
     mounted() {
+        const readyCta = this.isSuperLibrarian ? DO_MERGE : REQUEST_MERGE
         this.$watch(
             '$refs.mergeTable.merge',
             (new_value, old_value) => {
-                if (new_value && new_value !== old_value) this.mergeStatus = 'Do Merge';
+                if (new_value && new_value !== old_value) this.mergeStatus = readyCta;
             }
         );
     },
@@ -64,23 +88,32 @@ export default {
             const { record: master, dupes, editions_to_move, unmergeable_works } = this.$refs.mergeTable.merge;
 
             this.mergeStatus = 'Saving...';
-            try {
-                if (unmergeable_works.length)
-                {
-                    throw new Error(`Could not merge: ${unmergeable_works.join(', ')} has more than ${DEFAULT_EDITION_LIMIT} editions.`);
-                }
-                const r = await do_merge(master, dupes, editions_to_move, this.mrid);
-                this.mergeOutput = await r.json();
-                if (this.mrid) {
-                    await update_merge_request(this.mrid, 'approve', this.comment)
-                } else {
-                    const workIds = [master.key].concat(Array.from(dupes, item => item.key))
-                    await createMergeRequest(workIds)
+            if (this.isSuperLibrarian) {
+                // Perform the merge and create new/update existing merge request
+                try {
+                    if (unmergeable_works.length)
+                    {
+                        throw new Error(`Could not merge: ${unmergeable_works.join(', ')} has more than ${DEFAULT_EDITION_LIMIT} editions.`);
+                    }
+                    const r = await do_merge(master, dupes, editions_to_move, this.mrid);
+                    this.mergeOutput = await r.json();
+                    if (this.mrid) {
+                        await update_merge_request(this.mrid, 'approve', this.comment)
+                    } else {
+                        const workIds = [master.key].concat(Array.from(dupes, item => item.key))
+                        await createMergeRequest(workIds)
+                    }
+                } catch (e) {
+                    this.mergeOutput = e.message;
+                    this.mergeStatus = this.isSuperLibrarian() ? DO_MERGE : REQUEST_MERGE;
+                    throw e;
                 }
-            } catch (e) {
-                this.mergeOutput = e.message;
-                this.mergeStatus = 'Do Merge';
-                throw e;
+            } else {
+                // Create a new merge request with "pending" status
+                const workIds = [master.key].concat(Array.from(dupes, item => item.key))
+                const splitKey = master.key.split('/')
+                const primaryRecord = splitKey[splitKey.length - 1]
+                await createMergeRequest(workIds, primaryRecord, 'create-pending', this.comment)
             }
             this.mergeStatus = 'Done';
         },

openlibrary/components/MergeUI/MergeTable.vue

@@ -82,7 +82,8 @@ export default {
     },
     props: {
         olids: Array,
-        show_diffs: Boolean
+        show_diffs: Boolean,
+        primary: String
     },
     asyncComputed: {
         async records() {
@@ -93,8 +94,21 @@ export default {
             const records = _.orderBy(
                 await Promise.all(olids_sorted.map(fetchRecord)),
                 record => record.type.key, 'desc');
+
+            // Ensure that primary record is the first record
+            if (this.primary) {
+                const primaryKey = `/works/${this.primary}`
+                const primaryIndex = records.findIndex(elem => elem.key === primaryKey)
+                if (primaryIndex > 0) {
+                    const primaryRecord = records[primaryIndex]
+                    records.splice(primaryIndex, 1)
+                    records.unshift(primaryRecord)
+                }
+            }
+
             this.master_key = records[0].key
             this.selected = _.fromPairs(records.map(record => [record.key, record.type.key.includes('work')]));
+
             return records;
         },
 

openlibrary/components/MergeUI/utils.js

@@ -164,15 +164,18 @@ export function update_merge_request(mrid, action, comment) {
 }
 
 /**
- * Composes and POSTs a merge request with status "Merged"
+ * Composes and POSTs a new merge request.
  *
  * @param {Array<string>} workIds Un-normalized work OLIDs
+ * @param {string} primaryRecord The record in which to merge other records
+ * @param {'create-merged'|'create-pending'} action Determines the status code of the new request
+ * @param {string} comment Optional comment from request submitter
  *
  * @returns {Promise<Response>}
  */
-export function createMergeRequest(workIds) {
+export function createMergeRequest(workIds, primaryRecord, action = 'create-merged', comment = null) {
     const normalizedIds = prepareIds(workIds).join(',')
-    return createRequest(normalizedIds, 'create-merged', REQUEST_TYPES['WORK_MERGE'])
+    return createRequest(normalizedIds, action, REQUEST_TYPES['WORK_MERGE'], comment, primaryRecord)
 }
 
 /**

openlibrary/plugins/openlibrary/js/dialog.js

@@ -34,23 +34,27 @@ export function confirmDialog(callback, options) {
 function initConfirmationDialogs() {
     const CONFIRMATION_PROMPT_DEFAULTS = { autoOpen: false, modal: true };
     $('#noMaster').dialog(CONFIRMATION_PROMPT_DEFAULTS);
-    $('#confirmMerge').dialog(
-        $.extend({}, CONFIRMATION_PROMPT_DEFAULTS, {
-            buttons: {
-                'Yes, Merge': function() {
-                    const commentInput = document.querySelector('#author-merge-comment')
-                    if (commentInput.value) {
-                        document.querySelector('#hidden-comment-input').value = commentInput.value
+
+    const $confirmMerge = $('#confirmMerge')
+    if ($confirmMerge.length) {
+        $confirmMerge.dialog(
+            $.extend({}, CONFIRMATION_PROMPT_DEFAULTS, {
+                buttons: {
+                    'Yes, Merge': function() {
+                        const commentInput = document.querySelector('#author-merge-comment')
+                        if (commentInput.value) {
+                            document.querySelector('#hidden-comment-input').value = commentInput.value
+                        }
+                        $('#mergeForm').trigger('submit');
+                        $(this).parents().find('button').attr('disabled','disabled');
+                    },
+                    'No, Cancel': function() {
+                        $(this).dialog('close');
                     }
-                    $('#mergeForm').trigger('submit');
-                    $(this).parents().find('button').attr('disabled','disabled');
-                },
-                'No, Cancel': function() {
-                    $(this).dialog('close');
                 }
-            }
-        })
-    );
+            })
+        );
+    }
     $('#leave-waitinglist-dialog').dialog(
         $.extend({}, CONFIRMATION_PROMPT_DEFAULTS, {
             width: 450,

openlibrary/plugins/openlibrary/js/ile/utils/SelectionManager/SelectionManager.js

@@ -1,9 +1,7 @@
 // @ts-check
 import $ from 'jquery';
 import { move_to_work, move_to_author } from '../ol.js';
-import { PersistentToast } from '../../../Toast.js';
 import './SelectionManager.less';
-import { createRequest, REQUEST_TYPES } from '../../../merge-request-table/MergeRequestService'
 
 /**
  * The SelectionManager is responsible for making things (e.g. books in search results,
@@ -72,18 +70,12 @@ export default class SelectionManager {
 
         const actions = SelectionManager.ACTIONS.filter(a => (
             a.applies_to_type === provider.singular &&
-            (a.multiple_only ? count > 1 : count > 0) &&
-            a.elevated_permissions === this.canMerge
+            (a.multiple_only ? count > 1 : count > 0)
         ));
         const items = this.getSelectedItems().sort(this.collator.compare);
-        const target = this.canMerge ? 'target="_blank"' : ''
 
         for (const action of actions) {
-            const link = $(`<a ${target} href="${action.href(items)}">${action.name}</a>`)
-            $('#ile-drag-actions').append(link);
-            if (action.click_listener) {
-                link.on('click', action.click_listener)
-            }
+            $('#ile-drag-actions').append($(`<a target="_blank" href="${action.href(items)}">${action.name}</a>`));
         }
 
         if (selected) {
@@ -276,77 +268,22 @@ SelectionManager.ACTIONS = [
         multiple_only: true,
         name: 'Merge Works...',
         href: olids => `/works/merge?records=${olids.join(',')}`,
-        elevated_permissions: true,
-    },
-    {
-        applies_to_type: 'work',
-        multiple_only: true,
-        name: 'Request Merge...',
-        href: olids => `/works/merge?records=${olids.join(',')}`,
-        elevated_permissions: false,
-        click_listener: evt => {
-            evt.preventDefault()
-            const comment = prompt('(Optional) Are there specific details our librarians should note about this Merge Request?')
-            if (comment !== null) {
-                const href = document.querySelector('#ile-drag-actions').children[0].href
-                const url = new URL(href)
-                const params = new URLSearchParams(url.search)
-                const records = params.get('records')
-                createMergeRequest(records, REQUEST_TYPES['WORK_MERGE'], comment)
-            }
-        }
     },
     {
         // Someday, anyways!
         applies_to_type: 'edition',
         multiple_only: true,
         name: 'Merge Editions...',
         href: olids => `/works/merge?records=${olids.join(',')}`,
-        elevated_permissions: true,
-    },
-    {
-        applies_to_type: 'author',
-        multiple_only: true,
-        name: 'Merge Authors...',
-        href: olids => `/authors/merge?${olids.map(olid => `key=${olid}`).join('&')}`,
-        elevated_permissions: true,
     },
     {
         applies_to_type: 'author',
         multiple_only: true,
         name: 'Merge Authors...',
         href: olids => `/authors/merge?${olids.map(olid => `key=${olid}`).join('&')}`,
-        elevated_permissions: false,
-        click_listener: evt => {
-            evt.preventDefault()
-            const comment = prompt('(Optional) Are there specific details our librarians should note about this merge request?')
-            if (comment !== null) {
-                const href = document.querySelector('#ile-drag-actions').children[0].href
-                const olids = href.match(/OL(\d)+A/g)
-                createMergeRequest(olids.join(','), REQUEST_TYPES['AUTHOR_MERGE'], comment)
-            }
-        }
     },
 ];
 
-async function createMergeRequest(olids, type, comment) {
-    return createRequest(olids, 'create-pending', type, comment)
-        .then(result => result.json())
-        .then(data => {
-            let message;
-            if (data.status === 'ok') {
-                const username = document.querySelector('body').dataset.username
-                window.ILE.reset()
-                message = `Merge request submitted! View request <a href="/merges?submitter=${username}#mrid-${data.id}" target="_blank">here</a>.`
-            } else {
-                message = data.error
-            }
-            new PersistentToast(message, 'light-yellow').show()
-        })
-        .catch(function() {
-            new PersistentToast('Merge request failed. Please try again in a few moments.', 'light-yellow').show()
-        })
-}
 
 function sigmoid(x) {
     return Math.exp(x) / (Math.exp(x) + 1);

openlibrary/plugins/openlibrary/js/merge-request-table/MergeRequestService.js

@@ -4,7 +4,7 @@ export const REQUEST_TYPES = {
     AUTHOR_MERGE: 2
 }
 
-export async function createRequest(olids, action, type, comment = null) {
+export async function createRequest(olids, action, type, comment = null, primary = null) {
     const data = {
         rtype: 'create-request',
         action: action,
@@ -14,6 +14,9 @@ export async function createRequest(olids, action, type, comment = null) {
     if (comment) {
         data['comment'] = comment
     }
+    if (primary) {
+        data['primary'] = primary
+    }
 
     return fetch('/merges', {
         method: 'POST',

openlibrary/plugins/openlibrary/js/merge.js

@@ -3,10 +3,13 @@ import { declineRequest } from './merge-request-table/MergeRequestService';
 export function initAuthorMergePage() {
     $('#save').on('click', function () {
         const n = $('#mergeForm input[type=radio]:checked').length;
+        const confirmMergeButton = document.querySelector('#confirmMerge')
         if (n === 0) {
             $('#noMaster').dialog('open');
-        } else {
+        } else if (confirmMergeButton) {
             $('#confirmMerge').dialog('open');
+        } else {
+            $('#mergeForm').trigger('submit')
         }
         return false;
     });
@@ -40,12 +43,14 @@ export function initAuthorMergePage() {
 
 function initRejectButton() {
     const rejectButton = document.querySelector('#reject-author-merge-btn')
-    rejectButton.addEventListener('click', function() {
-        rejectMerge()
-        rejectButton.disabled = true
-        const approveButton = document.querySelector('#save')
-        approveButton.disabled = true
-    })
+    if (rejectButton) {
+        rejectButton.addEventListener('click', function() {
+            rejectMerge()
+            rejectButton.disabled = true
+            const approveButton = document.querySelector('#save')
+            approveButton.disabled = true
+        })
+    }
 }
 
 function rejectMerge() {