ensure MultipleChooserPanel selecting unique objects

[elhussienalmasri]

Fixes #10496

one solution for this issue is disable the duplicate objects or checkboxes, as shown below:

another solution instead of disable the already selected objects, just not add the duplicate objects , when the user add
duplicate object , this object not added to the selection objects. I can update the PR , if this solution is better.

[squash-labs]

Manage this branch in Squash

Test this branch here: https://elhussienalmasriensure-multipl-d6g25.squash.io

[gasman]

This is way too brittle - we can't assume that any numeric hidden input we find inside an InlinePanel child element is an object ID for the chooser we're interested in. Even if it did work, InlinePanel shouldn't be digging in to the internals of the forms it's managing to this extent.

[elhussienalmasri]

Thanks for feedback , this hidden input hold the unique ids of "MultipleChooserPanel" objects or checkboxes,
so what is your suggestions , may I put the unique ids in another element but the unique ids already in the hidden inputs ,so please tell me what must I do.
by the way I agree

we can't assume that any numeric hidden input we find inside an InlinePanel child element is an object ID for the chooser we're interested in

so may I put another unique attribute to the hidden input so its unique from the other hidden inputs.

[gasman]

I'd say this logic doesn't belong in InlinePanel, because there's no expectation that an InlinePanel will contain any choosers at all (for example, it can be a list of external links consisting of URL and label fields). If you put this function in MultipleChooserPanel instead, you can identify the relevant chooser widget by its id/name (opts.chooserFieldName) and retrieve its value through the chooserWidgetFactory object, just like we do when setting the widget's value. (Maybe it will help to add a utility method to InlinePanel for iterating over the active child forms - I see that this.formsElt.children(':not(.deleted)') appears a lot in the InlinePanel code, and it would be nice to avoid that pattern leaking into MultipleChooserPanel too.)

[elhussienalmasri]

Thanks, I will try to implement this approach.

[gasman]

Another thing - as per #10496 (comment), we only want to enable this behaviour when we're sure that choosing duplicates should not be allowed. Ideally, it would do this by looking for a unique constraint on the model - but as a first pass, an allow_duplicates flag on MultipleChooserPanel (defaulting to True) would do the job.

[elhussienalmasri]

depend on the suggestions, I did :

• move the "getChoiceSelectIds " method to "MultipleChooserPanel".
• set another unique attribute to the hidden input to make it unique.
• add "allow_duplicates" flag to "MultipleChooserPanel".

some fails because I added attribute to hidden input, I will work on this fails with unit tests. if I miss something regarding suggestions or this PR need any modification, please let me know.

[gasman]

• set another unique attribute to the hidden input to make it unique.

Please use opts.chooserFieldName / chooserWidgetFactory for this as I suggested.

Firstly, there is no guarantee that the chooser linked to MultipleChooserPanel's chooser_field_name is the only chooser in the panel.

Secondly - and more importantly - a change such as adding the choice-select-val-id attribute really hurts the maintainability of the code. In a project the size of Wagtail, it's really important to split things into independent components that don't need knowledge of the internal details of other components, so that you don't have to keep the entire codebase in your head all the time. Adding the choice-select-val-id attribute might be the most direct way to solve the problem you have right now, but it means that MultipleChooserPanel is relying on knowledge of an internal detail of ChooserWidget that isn't used anywhere else. Anyone working on ChooserWidget in future will have to constantly keep thinking "how will my changes affect MultipleChooserPanel?" Or, if they don't know about MultipleChooserPanel, they'll notice that choice-select-val-id isn't used anywhere in ChooserWidget's code and remove it, and not realise that they've broken MultipleChooserPanel. (This isn't so bad if MultipleChooserPanel has adequate test coverage so that we can see when it breaks. But are you confident that we have enough test coverage here to do that...?)

With chooserWidgetFactory, we have a reliable, well-documented way to get values in and out of a chooser - we should use it, rather than creating a new mechanism just for this one case.

[elhussienalmasri]

Great, I will update this PR soon.

[elhussienalmasri]

yes make the code or the components independent is better .I updated the PR, so please let me know, if it work
as expected or not.