Node Queue position duplication can cause accidental deletion of queue entries

I don't consider this a 'critical' bug, though it's definitely an annoying bug. Also, I prefer titles that describe the problem.

position should not be duplicated for a given qid; if it is, then that is a bug and it should be fixed. The code doesn't use NID as the primary identifier because it doesn't know what the NID is in all cases.

If your tests indicate that position can be duplicated, do you know how? That's the area that needs to be investigated.

In order to use an update I'd have to set the position to some temporary value to ensure no duplication. It may be a better solution, I'll have to run through it. When I was writing the module I had originally not meant to use delete but ended up with it because the other method used more queries.

Oh yes, that snapshot shows there is definitely a bug in there with respect to setting the position flag. I looked but I couldn't see a glaring error, but it is clearly there. I'll have to see if I can do it.

Do the various QIDs that have a bunch of 3s in the position table have anything in common?

I am blind. I looked at every one of those queries multiple times, but...well, I knew what I was supposed to be seeing and only saw that, I guess.

Fix put in and tested in both 4.6 and 4.7 versions. cron will run the packaging sometime today. Embarassment will last awhile.

Hi,
I am using 7.x-2.0-beta1 , the problem is still exists in this version. When I select 1 as position in two drop-downs(as in attached image) , one item gets deleted on submit.

I fixed the bug of #11 by applying this patch .
Please review.

Found One more issue :( The code at line:880
form_set_error($nid . '][position', t('Duplicate position value.'));

is not highlighting as error (border:red) to the select lists containing duplicate values.
It should be as attached image . going to submit another patch for it.

This patch is to fix issue in both #11 and #13

this issue has been fixed by previous patch

I just ran into this bug because the Javascript in nodequeue_dragdrop.js -> nodequeueUpdateNodePositions() was not consistently changing the selected options, which caused duplicate positions, and thus deleted queue entries. Here's the code I added to the patch in #14:

diff --git a/nodequeue_dragdrop.js b/nodequeue_dragdrop.js
index 406b76f..43e673a 100644
--- a/nodequeue_dragdrop.js
+++ b/nodequeue_dragdrop.js
@@ -162,6 +162,7 @@ function nodequeueUpdateNodePositions(table_id) {
 
   $('#' + table_id + ' tr').filter(":visible").find('.node-position').each(function(i) {
     $(this).val(size);
+    $(this).find("option[value='" + size + '"]').attr('selected', 'selected');
     reverse ? size-- : size++;
   });
 }

Actually, there's one more change. Too many elements were being included in the find():

diff --git a/nodequeue_dragdrop.js b/nodequeue_dragdrop.js
index 406b76f..cbf8f97 100644
--- a/nodequeue_dragdrop.js
+++ b/nodequeue_dragdrop.js
@@ -160,8 +160,9 @@ function nodequeueUpdateNodePositions(table_id) {
   var reverse = Drupal.settings.nodequeue.reverse[table_id.replace(/-/g, '_')];
   var size = reverse ? $('#' + table_id + ' .node-position').size() : 1;
 
-  $('#' + table_id + ' tr').filter(":visible").find('.node-position').each(function(i) {
+  $('#' + table_id + ' tr').filter(":visible").find('select.node-position').each(function(i) {
     $(this).val(size);
+    $(this).find("option[value='" + size + '"]').attr('selected', 'selected');
     reverse ? size-- : size++;
   });
 }

If someone can mark this RTBC after testing it, I can commit the patch.

@fizk, do you have the steps to reproduce? It's all gotten a bit convoluted over 8 years so providing steps might be the ticket to getting this reviewed and fixed. I'll be your first tester. :)

@othermachines Thanks :) Try to create a queue with three items. Arrange them in a specific order, save that, then rearrange them in another order. Notice that it won't save according to the order you wanted.

I can't seem to reproduce the delete behaviour when using drag-and-drop, only with row weights. I've revised the steps to reflect this.

Regarding the problem reported in #17 - I couldn't reproduce this at all. Maybe we need separate steps?

Steps to reproduce

1. Create queue with 3 items;

Result:
- First item
- Second item
- Third item

2. Using row weights (not drag-and-drop), change position of third item to [1];

3. Save.

Result:
- Third item
- Second item

First item is deleted.

Patch in #19 needs an isset() check to avoid an "Undefined index" error when adding the first node to the queue.

function nodequeue_arrange_subqueue_form_validate($form, &$form_state) {
  $positions = array();
  
  // We only need to validate position on queues containing >= 1 node.
  if (isset($form_state['values']['nodes'])) {
    foreach ($form_state['values']['nodes'] as $nid => $element) {
      if (is_numeric($nid) && is_numeric($element['position'])) {
        $positions[$nid] = $element['position'];
      }
    }
    if (count(array_unique($positions)) < count($positions)) {
      $seen = array();
      foreach ($positions as $nid => $position) {
        if (isset($seen[$position])) {
          form_set_error('nodes][' . $nid . '][position', t('Duplicate position value.'));
        }
        $seen[$position] = TRUE;
      }
    }
  }
}

As mentioned above, I couldn't reproduce the problem in nodequeue_dragdrop.js, so couldn't test. Sorry. :-/

Committed with the isset(). Thanks!