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

Broken checkboxes in MultipleChooserPanel modal when USE_THOUSAND_SEPARATOR is True #11137

Closed
wants to merge 4 commits into from

Conversation

codesmith25103
Copy link
Contributor

@codesmith25103 codesmith25103 commented Oct 26, 2023

Fixes #...
Issue Number #11134
I attempt to solve the issue as it was mentioned in PR. I have used the filter "|unlocalize" at a correct place to solve this issue

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
    • Please list which assistive technologies 3 you tested:
  • For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

@squash-labs
Copy link

squash-labs bot commented Oct 26, 2023

Manage this branch in Squash

Test this branch here: https://codesmith25103main-agok7.squash.io

@@ -1,4 +1,4 @@
{% load i18n %}
<td {% if column.classname %}class="{{ column.classname }}"{% endif %}>
<input type="checkbox" id="chooser-modal-select-{{ value }}" data-multiple-choice-select name="id" value="{{ value }}" title="{% blocktrans trimmed with title=instance %}Select {{ title }}{% endblocktrans %}">
<input type="checkbox" id="chooser-modal-select-{{ value }}" data-multiple-choice-select name="id" value="{{ value|unlocalize }}" title="{% blocktrans trimmed with title=instance %}Select {{ title }}{% endblocktrans %}">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other occurrence of value (in "chooser-modal-select-{{ value }}") needs updating too. Have also just noticed that there's another template that needs the same fixes - wagtail/admin/templates/wagtailadmin/chooser/tables/page_checkbox_select_cell.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing. I will make required changes.

@codesmith25103
Copy link
Contributor Author

I have made required changes. Please review it

@zerolab zerolab changed the title Issue Number 11134 Broken checkboxes in MultipleChooserPanel modal when USE_THOUSAND_SEPARATOR is True Oct 26, 2023
@lb-
Copy link
Member

lb- commented Oct 26, 2023

It might be good to have a unit test supporting this also, so that it doesn't break again in the future.

Copy link
Collaborator

@gasman gasman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently failing when I test it on the bakerydemo site (by editing any blog page, and clicking the "Add author" button which should open the chooser popup) because the template isn't loading the l10n tag library (which is required for the unlocalize filter).

I would have expected the existing unit tests to catch this error, so it seems that we're lacking some test coverage for this.

@codesmith25103
Copy link
Contributor Author

So do I have to write unit test to catch this error?

@lb- lb- added status:Needs Tests component:UI localization l10n & i18n for admin interface only component:Choosers Modal choosers for Page, Snippet and other models labels Oct 27, 2023
@codesmith25103
Copy link
Contributor Author

I have added l10n tag in both the files. Hope it will resolve the issue.

@lb-
Copy link
Member

lb- commented Oct 27, 2023

@codesmith25103 you may want to add a test for this change.

  • wagtail/admin/tests/test_edit_handlers.py - see TestMultipleChooserPanel - this is more for the panel itself, probably not the view that responds with the checkboxes.
  • For the multiple chooser response view, I cannot see that we have a test for this view specifically. It probably needs to go here wagtail/admin/tests/test_page_chooser.py. In the class TestChooserBrowse a new test that requests with multiple=1 should get you started.

The view is located in wagtail/admin/views/chooser.py (see BrowseView) and the URL is set up in wagtail/admin/urls/__init__.py.

class TestChooserBrowse(WagtailTestUtils, TestCase):

    @override_settings(USE_THOUSAND_SEPARATOR=True)
    def test_multiple_chooser_view(self):
        response = self.get({"multiple": "1"})  # something like this
        self.assertEqual(response.status_code, 200)
        self.assertTemplateUsed(response, "wagtailadmin/chooser/browse.html")
        # check that the values are correct
        # We may need to add a page that has an id of over 1000
        # we will also need to test this with the setting turned off
        # test data is set in wagtail/test/testapp/fixtures/test.json but we can also add a page on the fly in this test

Hopefully that helps as a starting point, a similar test will be needed for the generic multiple chooser but maybe get the page one working first.

@lb- lb- requested review from lb- and removed request for lb- November 10, 2023 22:17
@lb-
Copy link
Member

lb- commented Nov 28, 2023

@codesmith25103 are you OK to create a unit test for this?

@codesmith25103
Copy link
Contributor Author

Hey @lb-, I apologize for the delay. Due to some health and family issues, I wasn't able to complete it. However, I have already started working on it and will update you soon.

@lb-
Copy link
Member

lb- commented Nov 29, 2023

No problems @codesmith25103 - I hope you and your family are OK.

@lb-
Copy link
Member

lb- commented Jan 20, 2024

Thanks @codesmith25103 looks like a new PR has taken this and added unit tests.
#11490

We will ensure we mention you in the release notes, thanks for getting this started.

@lb- lb- closed this Jan 20, 2024
lb- added a commit to rohitsrma/wagtail that referenced this pull request Jan 20, 2024
lb- added a commit that referenced this pull request Jan 20, 2024
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 component:UI localization l10n & i18n for admin interface only status:Needs Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants