IntegrityError translationimportqueueentry__entry_per_importer__unq Product+edit-people

[Curtis Hovey]

OOPS-1714L464 show that IntegrityError: duplicate key value violates unique constraint "translationimportqueueentry__entry_per_importer__unq" was cause when the user tried to set the project owner to the same team as the project driver (elementaryart) This data is fine and the change calls self.updateContextFromData(data).

The product-edit-people-view.txt test indicates that the translationimportqueue.importer is reassigned. This may be wrong because I believe the entry is already owned by the driver.

[Curtis Hovey (sinzui)]

This certainly is a rosetta rule causing the error, but I cannot see where the transfer happens. I image we want to check that the change is does not create a duplicate before it happens. Maybe the rule is not needed since the driver alreayd appears to own the entry.

[Curtis Hovey (sinzui)]

This was caused in lp/registry/model/product.py Product._setOwner does the reassign. I think the series and release reassignment is no longer needed because the security checkers ignore the object owner in favour of the product owner or driver. The EditTranslationImportQueueEntry checker states that the product owner is used, not importer, so I do no see why this method does any of this work.

[Curtis Hovey (sinzui)]

Hi Jeroen.

I picked you at random from the Rosetta team. Why does Product._setOwner need to do this:
            import_queue = getUtility(ITranslationImportQueue)
            for entry in import_queue.getAllEntries(target=self):
                if entry.importer == old_owner:
                    removeSecurityProxy(entry).importer = new_owner

The security checkers give the product owner power over all the subordinate objects, so I do not see why the entry.import needs to change.

We can prevent the oops by checking that the new owner is not already an importer, but I suspect the work this method is doing is pointless--being the owner of a series or a release does not give someone power and we do not show it in the UI.

[Curtis Hovey (sinzui)]

We can verify this bug is fixed on edge by setting /nautilus-elementary owner to ~elementaryart. I will ask the project owner when we think the fix is ready.

[ammonkey (am-monkeyd)]

i don't understand everything here, but if you need to change nautilus-elementary owner to ~elementaryart, indeed you can.

[Данило Шеган (danilo)]

Curtis, I can only guess why we are doing this: I assume it's some old style code that allowed files to be imported. Once stuff gets into the import queue, it's already passed the checks for appropriate permissions. They should stilll be imported, and I'd even argue that it's wrong to reassign the owner of the import queue entries.

So, my take: get rid of that code.

(FWIW, QA might not be as easy if all files end up being imported and subsequently removed [automatically] from the queue: I see there are a few unapproved translations still, so you should be ok)

[Jeroen T. Vermeulen (jtv)]

I have a vague recollection that the problem wasn't so much the security choice as an oh-dear-what-do-we-do-now situation caused by contradictory security information. I wouldn't delete the code without seeing what happens.

Nominally, yes, by the time the upload is on the import queue the entry has passed security checks and there's nothing against processing it. But in practice, my vague recollection says, something blows up unexpectedly at a later stage because the uploader isn't allowed to make the changes that the import will make in their name. And we circumvented that by transferring uploadership.

One thing that would fix this is the long-desired ability to import unprivileged uploads as suggestions. The uploads would do something sensible automatically. It'll be easier to do in the Recife model though, which at the moment isn't quite finished.

It's tempting to do something more radical, like marking the entries as Blocked or Deleted, but that's not as easy as it sounds. The change of ownership doesn't _necessarily_ mean that the uploader has lost upload privileges, so we'd have to check.

One option that looks quite attractive to me now is: create a "forbidden" upload in the staging database, process it, and see where it fails. Then make sure we handle that failure, marking the queue entry as Failed. Once we start supporting suggestions imports, we'll slip naturally into more pleasant behavior where the import can succeed but does not affect which translations are current.

[Curtis Hovey (sinzui)]

Hi Jeroen.

I am inclined to remove the reassignment code. The code is faulty in this case because it assumes the owner is responsible for the entry, but it is in fact the drivers, who did not change. Most cases of project ownership are from user to user's team; the user has the same permissions.

In the very rare case where the user gifts the project to another user or the registry admins, I think you are saying the lp acts as the uploader to update the import, maybe it should be acting as the project owner for the purposes of security. I say this because it is not clear in the code that once the entry is accepted, we intend to process it regardless of who who owns the project.

If we must keep the _setOwner, then I will still remove the release and series code, and I think the entry loop should only change if the old own has less power and the entry is owned by that user.

[Curtis Hovey (sinzui)]

My reading of the tests and code imply an imported pofile may not be accepted, they messages will be set as suggestions.

How would I setup a test on staging? I want to land my branch that removes the code, Then test all is well.

[Jeroen T. Vermeulen (jtv)]

This reading is not entirely correct.

If a file is accepted, an attempt is made to import it. If a file is not accepted, it never goes onto the queue. It is possible for _individual messages_ in a translation to be demoted to suggestions, but only insofar as they conflict with concurrent changes.

The code that dictates this behaviour is locked up in an enormous primeval method that we have sworn a sacred oath not to disturb until the Recife feature branch is ready to replace it altogether.

[Curtis Hovey (sinzui)]

Hi Danilo and Jeroen.

I understand your fear. I am blocked though. I could say this is a rosetta bug and stop working on it. but I think there is something missing from discussion: This is what I see:

An owning-user has an import, a driving-team has an import. The code wrongly assumes the user will loose permission so attempts the impossible by giving the driving team two entries in the queue.

Maybe the problem is that the queue is allowing two entries; the constraint importer + path + productseries should be path + productseruies. Probably not.

Maybe the users entry should be deleted in this case.

Maybe there should be a guard on Product.owner that blocks renaming while there is an entry in the queue.

Why isn't there are guard for changing the driver? Would their import break if the driver was changed?

What happens if I loose permission by leaving a team while my entry is in the queue?

These later two points will happen in Launchpad more often then a user assigning their project to another user or unrelated team, but I do not see an oopses reporting a permission problem in rosetta.

[Jeroen T. Vermeulen (jtv)]

A thought: we made the import script a lot more robust over time. When we introduced this weirdness, it was probably urgently needed to keep the script running. It no longer should be, even if some imports fail.

So we could just take the risk: remove the code that tries to transfer the queue entries, and see what happens. Some imports may fail (though some won't) and they should just go into the Failed state with error information available in the UI. That may even be a good approximation of what users will want for the problem case. It shouldn't bring the script to its knees any more.

[Launchpad QA Bot (lpqabot)]

Fixed in stable r11632 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/11632>.

[Curtis Hovey (sinzui)]

Hi ammonkey. Your team is not the maintainer of your project.