Design issue Concurrent editing of ads reverts one admin's changes

PaulB

PaulB

Well-known member
Affected version
2.2.7 Patch 1
If two admins are working on ads simultaneously, one will revert the other's changes. For example:
  1. Alice opens the ads page.
  2. Bob opens the ads page.
  3. Alice creates a new ad.
  4. Alice's ad is now enabled.
  5. Bob toggles the visibility of an unrelated ad.
  6. Alice's ad is now disabled because Bob's form data doesn't contain that ad.
This is, of course, undesirable. It can even happen with a single admin who happens to leave multiple tabs open.

Here's another example:
  1. Alice opens the ads page.
  2. Bob opens the ads page.
  3. Alice toggles the visibility of an ad.
  4. Bob toggles the visibility of another ad.
  5. The action performed in step #3 has been reverted.
 
I mean this can happen on pretty much any form submission in the software. There likely is solutions but they are unlikely to be simple to implement so I recommend posting a suggestion for consideration in the future.
 
Chris D said:
I mean this can happen on pretty much any form submission in the software. There likely is solutions but they are unlikely to be simple to implement so I recommend posting a suggestion for consideration in the future.
Click to expand...
It's not obvious that the form submits every toggle on the page, though. I expected that when clicking a toggle, it would just toggle that specific ad, not submit every toggle on the page. At the very least, it shouldn't disable ads for which no form entry exists. To the average person, it wouldn't even be obvious that it's a form--there's no submit button.
 
Admittedly I did miss the info about toggling so I was rash in my dismissal of it. Sorry about that. But to be clear, the behaviour you describe in your first example is not how it works. We do only toggle ads that are passed into the request and others are ignored.

PHP: 
$activeState = $this->request->filter($options['input'], 'array-bool');
$entities = $this->em->findByIds($identifier, array_keys($activeState));

$options['input'] here is the name of the input. It's usually active and will get the IDs of each checkbox on the current page, then what is fetched and toggled from there is based entirely on that. In other words if Bob has ads 1 and 2, and Alice has created ad 3 since Bob reloaded the page, only ads 1 and 2 will be changed.

Your second example is reproducible but my feelings of it still mostly stand. This is how we've been handling toggle elements as far back as XF 1.0 and it affects literally any list page that contains toggles. Changes here would be outside the scope of a bug report as it would require a fairly significant change of approach and not one we would be targeting for a bug fix release.
 
Chris D said:
We do only toggle ads that are passed into the request and others are ignored.
Click to expand...
Hmm, there may have been another compounding factor. I'll investigate further if it happens again. I assumed it had the same cause as the second repro, but clearly that's not the case.
 
@Chris D, it seems that when we save an ad, it gets disabled, as there's no "Active" checkbox there. Are you able to confirm whether that's a vanilla XF bug? It may be something on our end. This appears to be the cause of the first repro I mentioned.

Edit: I've been told we've confirmed this to be an issue on our end.
 
Last edited:
Chris D said:
That seems to make the most sense to me based on my testing and the code. Hope it's not too tricky to sort!
Click to expand...
Yup, just an overzealous regex in a template mod. That means the first repro is invalid and was almost certainly a result of a separate bug that isn't present in vanilla XF. Only the second repro stands and is a known caveat with how toggles in admin.php are handled.
 
Top Bottom