Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultipleChooserPanel UI does not respect max_num #11561

Open
dopry opened this issue Jan 31, 2024 · 17 comments · May be fixed by #11738
Open

MultipleChooserPanel UI does not respect max_num #11561

dopry opened this issue Jan 31, 2024 · 17 comments · May be fixed by #11738
Labels
component:Choosers Modal choosers for Page, Snippet and other models type:Bug

Comments

@dopry
Copy link
Contributor

dopry commented Jan 31, 2024

Issue Summary

When using a MultipleChooserPanel with a max_num of 3, I am able to check more than 3 options. You can confirm the selection and max_num is respected, but it looks like an arbitrary items from the selection up to max_num are saved.

image

I would expect my existing selections to be highlight and not to be able to select more than my existing selection.

Steps to Reproduce

  1. setup a multiple chooser panel and set max_num to 3.
  2. add values (select 2) and confirm selections
  3. add values again and select 7 items and confirm selection

Technical details

  • Python version: 3.10
  • Django version: 3.2.9
  • Wagtail version: 4.2.4

Working on this

Anyone can contribute to this. View our contributing guidelines, add a comment to the issue once you’re ready to start.

@dopry dopry added status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. type:Bug labels Jan 31, 2024
@gasman gasman removed the status:Unconfirmed Issue, usually a bug, that has not yet been validated as a confirmed problem. label Feb 2, 2024
@gasman
Copy link
Collaborator

gasman commented Feb 2, 2024

Related: #10754 (and to some extent #10496). These all stem from the fact that the chooser popup was designed to be a mechanism to get values into the panel, rather than being a representation of the panel's current state - however, that's evidently breaking user expectations, so accepting this as a valid bug.

@noobCoderVP
Copy link

Hello, May I work on this issue? Can anyone help me with the basic code setup to reproduce the issue?

@gasman
Copy link
Collaborator

gasman commented Feb 2, 2024

@noobCoderVP You're welcome to give it a go, but I don't think this is going to be a straightforward fix. You can replicate this on the bakerydemo project - the Authors panel on BlogPage is a MultipleChooserPanel, and you'll just need to add max_num=3 or similar to its definition.

@noobCoderVP
Copy link

Ok, I will try to fix it.

@noobCoderVP
Copy link

Hey @gasman, I was able to reproduce the issue on recipe page, When we select more than 3 authors, it selects the first 3 authors out of selected 4 authors.

image image

There is one more issue of duplicate authors, if we first select 1 author and again select the same author, name is duplicated in the authors list

image

I have found the code where we are using MultipleChooserPanel
image

I will try to fix both the issues, Let me know if there are any suggestions from your side

@gasman
Copy link
Collaborator

gasman commented Feb 6, 2024

@noobCoderVP The point about duplicates is covered in a separate issue: #10496. As mentioned there, there are situations where we do want to allow duplicates: for example, if you're building a slideshow of images with captions, and you want to allow the same image to appear multiple times (possibly with different captions). On that issue, I proposed that MultipleChooserPanel should only prevent selecting duplicates when a unique_together constraint exists on the relation.

This is also very relevant to the current issue. In the case where we do allow duplicates, it doesn't make sense for the chooser to show existing choices when reopening it, because then there's no way to indicate "add another copy of this item". (It also makes the max_num logic really messy: suppose we have a slideshow with max_num=5, and it initially contains images A, B, C, A. The chooser should allow selecting one more item; however, if you de-select A within the chooser, it will now be valid to select 3 new ones.)

Another consideration I didn't think about earlier: there's an open proposal (#9784) to change the behaviour of max_num in StreamField so that editors can exceed the maximum number of blocks while they are editing, but must bring it back under the limit before they save. If we go ahead with this change, it would be sensible to follow the same behaviour for InlinePanel / MultipleChooserPanel - in which case, the chooser should not enforce the maximum count, at least in the "allow duplicates" case.

@noobCoderVP
Copy link

Ohh, functionality is pretty complex! Anyways let's first fix the enforcement of max_num in MultipleChooserPanel, I will fix it and test it. Let's handle duplicate case later. I am currently using docker to run bakerydemo project, I wanted to know how should I change the source code of wagtail locally to get it reflected in the bakerydemo project?

@gasman
Copy link
Collaborator

gasman commented Feb 6, 2024

I don't use Docker for Wagtail development myself, but https://github.com/wagtail/docker-wagtail-develop should allow you to set up a development environment.

@noobCoderVP
Copy link

Hey @gasman, With your help finally I was able to setup the development environment, I have found the code that we need to change

image

I wanted to understand more about the frontend code, Do you have any article where they have mentioned the organization of frontend code and how it is used?

@noobCoderVP
Copy link

Hey @gasman I need a final help, I went through whole docs of Wagtail and saw how codebase works. I was able to understand the python side of the code with templates and models. I wanted to add a click event on checkbox selection which will prevent the selection of items more than the max_num. I wanted to know more about the code in client/src folder. What steps should I follow for that?

@noobCoderVP
Copy link

Basically the question is how do we render the frontend of what we provide in content_panels inside a model (RichTextField, MultipleChooserPanel, etc..). I am trying to figure it out by my side but looks very complex. I learned about telepath library which helps in data movement between python and javascript, it is also used in the MultipleChooserComponent.

image

This code looks to handle the opening and closing of multiple chooser modal, I wanted to know more about what chooserWidgetFactory is that is retrieved from telepath, formSetPrefix etc.. Let me know if you can help me.

@gasman
Copy link
Collaborator

gasman commented Feb 9, 2024

I did say up-front it probably wouldn't be a straightforward fix! You might be able to get some further pointers in the #new-contributors channel on the Wagtail Slack.

@noobCoderVP
Copy link

Yess, I have joined the channel. Will ask there. I knew this task is going to be tough but I enjoyed struggling and learnt a lot about the hardwork that you guyz have put to make Wagtail CMS. Model based admin panels is amazing!! I would have gone through whole codebase last 2-3 days. It's my first experience to understand such a huge codebase. @gasman you have helped a lot!! Thanks for that. I learnt a lot of things meanwhile. I will fix this issue at any cost and keep contributing...

@lb-
Copy link
Member

lb- commented Mar 17, 2024

I'd like to propose some alternatives to the PR #11738 from @elhussienalmasri

These ideas avoid baking in custom behaviour to the multiple chooser panel code but instead build on the other Stimulus controller code that already exists.

At some point we will want to start tackling the migration of InlinePanel and MultipleChooserPanel to Stimulus so the less bespoke behaviour we have in that code the better. Additionally, making our generic Stimulus controllers more usable, robust and flexible is going to pay dividends down the road.

Idea 1 - Use CountController (w-count)

This might be the simplest approach, we can leverage this controller to conditionally show a warning/label when the count of selected checkboxes reaches some threshold.

Either we show a warning when the count reaches the total (1a) or it exceeds it (1b). Another approach is that we show the counts of selection the entire time (1c).

There three options should not require any new JavaScript code, just the additional html for the message and data attributes to use the controller.

An example of how to get the controller connected to the form element. This will do the counting but you'll need to ensure the labels get set up. See the unit tests and other usage for examples.

wagtail/admin/templates/wagtailadmin/generic/chooser/chooser.html

<form action="{{ chosen_multiple_url }}" method="GET" data-multiple-choice-form data-controller="w-count" data-action="w-coun#count" data-w-count-find-value="input:checked" data-w-count-container-value="[data-multiple-choice-form]">

It may be worth making it easier for this controller to count things in the controlled element only. Maybe when the container value is not set it could use the controlled element instead.

Another option (1d) could be to allow for the labels to be updated when when the minValue has not been reached.

Another option (1e) might be to enhance the controller to allow a target to be disabled if the threshold is reached. In this way we could dynamically disable the submit button. Although this is usually bad practice and instead we would want to consider a a better approach where it's enabled but clicking it does not submit but instead shows or focuses on a warning.

Idea 2 - Use BulkController (w-bulk)

This controller provides ways to manage groups of checkbox selections, allowing shift-click to select multiple in a row. Also allows a button to allow bulk selection of toggling all items. Finally it supports adding a class to the element once a selection has been made.

It's doesn't, yet, support the ability to disable inputs based on some maximum number. Maybe, this controller could be enhanced to support this restriction and be used to solve this issue.

We would need to add a Target support for the actual checkbox inputs and consider edge cases such as shift+click scenarios and toggling all.


Note. We may end up wanting a combination of ideas to ensure we advise the user of their total selection compared to the max selection AND block the ability to go over the max selection.

@lb- lb- added the component:Choosers Modal choosers for Page, Snippet and other models label Mar 17, 2024
@elhussienalmasri
Copy link
Contributor

Great ideas @lb- , as I already raised PR , so I already have basic understand of the problem ,so I will continue work in this issue
on the another approach but after some days. but I don ,t know if I must display counter and also disabled the checkboxes
or one of them is enough like display counter like "RichTextField" has counter but not disabled when reach to max_num,
by the way I will make the two ways like this PR #11738 . and also not only the forms need be count but also the submitted
forms need to be count because the user may select the max_num in more than once, for example if "max_num = 3" the user
may choose two and submit the forms and after that select one. so if there a way for "Count Controller" approach
to count also the submitted forms. as I said I will continue to work on this issue but after some days.

@lb-
Copy link
Member

lb- commented Mar 17, 2024

Thanks @elhussienalmasri - it's a good point about the counting of elements that may not be uniquely available in the DOM.

See how you go with the suggestions above, I'll try to dig into this a bit more if I get the chance and update my comment above with a bit more code examples.

But as @gasman pointed out earlier, looks like this may be a tricky one.

@elhussienalmasri
Copy link
Contributor

elhussienalmasri commented Mar 17, 2024

yes I agree, this what make this issue tricky

it's a good point about the counting of elements that may not be uniquely available in the DOM.

plus we need to count the previous selected forms when user open the forms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Choosers Modal choosers for Page, Snippet and other models type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants