ronantakizawa/codereview-bench huggingface dataset:
Repository: wagtail/wagtail
File: wagtail/project_template/home/tests.py (Python)

Code before:
# home/tests.py

from django.test import TestCase
from wagtail.models import Page
from home.models import HomePage


class HomeSetUpTests(TestCase):
    """
    Tests steps needed by follow up tests
    """

    def test_root_create(self):
        Page.objects.get(pk=1)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title='Home')
        root_page.add_child(instance=self.homepage)


class HomeTests(TestCase):
    """
    Class for testing homepage logic
    """


Review comment (suggestion):
```suggestion
```

Do we need this comment? it does not look like we do this in any other template files.

Code after:
from django.urls import reverse
from home.models import HomePage

from wagtail.models import Page
from wagtail.test.utils import WagtailPageTestCase


class HomeSetUpTests(WagtailPageTestCase):
    """
    Tests for basic page structure setup and HomePage creation.
    """

    def test_root_create(self):
        root_page = Page.objects.get(pk=1)
        self.assertIsNotNone(root_page)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Tests for homepage functionality and rendering.
Repository: wagtail/wagtail
File: wagtail/project_template/home/tests.py (Python)

Code before:
# home/tests.py

from django.test import TestCase
from wagtail.models import Page
from home.models import HomePage


class HomeSetUpTests(TestCase):
    """
    Tests steps needed by follow up tests
    """

    def test_root_create(self):
        Page.objects.get(pk=1)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title='Home')
        root_page.add_child(instance=self.homepage)


class HomeTests(TestCase):
    """
    Class for testing homepage logic
    """

    def setUp(self):
        """

Review comment (suggestion):
I think we should try to use our documented testing approach in https://docs.wagtail.org/en/stable/advanced_topics/testing.html


```suggestion
from wagtail.test.utils import WagtailPageTestCase
```

Also, have a review of that page for some other tips/practice we recommend in our docs.

Code after:
from django.urls import reverse
from home.models import HomePage

from wagtail.models import Page
from wagtail.test.utils import WagtailPageTestCase


class HomeSetUpTests(WagtailPageTestCase):
    """
    Tests for basic page structure setup and HomePage creation.
    """

    def test_root_create(self):
        root_page = Page.objects.get(pk=1)
        self.assertIsNotNone(root_page)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Tests for homepage functionality and rendering.
    """
Repository: wagtail/wagtail
File: wagtail/project_template/home/tests.py (Python)

Code before:

    def test_root_create(self):
        root_page = Page.objects.get(pk=1)
        self.assertIsNotNone(root_page) 

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Class for testing homepage logic
    """

    def setUp(self):
        """
        Set up the testing environment.
        """

        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title='Home')
        root_page.add_child(instance=self.homepage)
        
    def test_your_test(self):
        """
        Tests if BlogIndexPage can be created.
        """

        raise NotImplementedError("The tests are not implemented yet.")

Review comment (suggestion):
Note, AI suggestion, but maybe we should do something else here to actually have a basic starting set of tests.

```suggestion
    def test_homepage_status_code(self):
        response = self.client.get(reverse('home'))
        self.assertEqual(response.status_code, 200)

    def test_homepage_template_used(self):
        response = self.client.get(reverse('home'))
        self.assertTemplateUsed(response, 'home/index.html')
```

Be sure to run this code yourself, ensuring the tests run for a new project.

Code after:
    """
    Tests for basic page structure setup and HomePage creation.
    """

    def test_root_create(self):
        root_page = Page.objects.get(pk=1)
        self.assertIsNotNone(root_page)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Tests for homepage functionality and rendering.
    """

    def setUp(self):
        """
        Create a homepage instance for testing.
        """
        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title="Home")
        root_page.add_child(instance=self.homepage)

    def test_homepage_status_code(self):
        response = self.client.get(reverse("home"))
        self.assertEqual(response.status_code, 200)

    def test_homepage_template_used(self):
        response = self.client.get(reverse("home"))
        self.assertTemplateUsed(response, "home/home_page.html")
Repository: wagtail/wagtail
File: wagtail/project_template/home/tests.py (Python)

Code before:
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Class for testing homepage logic
    """

    def setUp(self):
        """
        Set up the testing environment.
        """

        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title='Home')
        root_page.add_child(instance=self.homepage)
        
    def test_your_test(self):
        """
        Tests if BlogIndexPage can be created.
        """

        raise NotImplementedError("The tests are not implemented yet.")

Review comment (refactor):
I think we should choose to start with a set of tests that pass. It's possible it could cause confusion if the tutorial starting point does not pass.

I would recommend we actually remove this 'placeholder' test and just add a few more starting tests.

Code after:
        self.assertIsNotNone(root_page)

    def test_homepage_create(self):
        root_page = Page.objects.get(pk=1)
        homepage = HomePage(title="Home")
        root_page.add_child(instance=homepage)
        self.assertTrue(HomePage.objects.filter(title="Home").exists())


class HomeTests(WagtailPageTestCase):
    """
    Tests for homepage functionality and rendering.
    """

    def setUp(self):
        """
        Create a homepage instance for testing.
        """
        root_page = Page.objects.get(pk=1)
        self.homepage = HomePage(title="Home")
        root_page.add_child(instance=self.homepage)

    def test_homepage_status_code(self):
        response = self.client.get(reverse("home"))
        self.assertEqual(response.status_code, 200)

    def test_homepage_template_used(self):
        response = self.client.get(reverse("home"))
        self.assertTemplateUsed(response, "home/home_page.html")
Repository: wagtail/wagtail
File: wagtail/admin/utils.py (Python)

Code before:

def get_keyboard_key_labels_from_request(request):
    """
    Returns an instance of SimpleNamespace based on the user's keyboard layout
    based on the User-Agent header in the request.

    These are intentionally not translated, as they are key labels that are assumed
    to be consistent across all languages.
    """

    user_agent = request.headers.get("User-Agent", "")
    is_mac_os = re.search(r"Mac|iPod|iPhone|iPad", user_agent)

    labels = {
        "ALT": "⌥" if is_mac_os else "Alt",
        "CMD": "⌘" if is_mac_os else "Ctrl",
        "CTRL": "^" if is_mac_os else "Ctrl",
        "DEL": "Delete",
        "ENTER": "Return" if is_mac_os else "Enter",
        "ESC": "Esc",
        "SHIFT": "Shift",
        "TAB": "Tab",
    }

    return SimpleNamespace(**labels)


Review comment (suggestion):
Can we please also add a 'mod' (modifier) name here, this is commonly used by libraries such as mousetrap (JS) to refer to the CMD or Ctrl differences.

Then, can we replace the usage in the template tag with `.MOD`.

We can still have two variations in the output, but that way the overall differences in the template tag code make a bit more sense.


```suggestion
        "CMD": "⌘" if is_mac_os else "Ctrl",
        "MOD": "⌘" if is_mac_os else "Ctrl",
```

Please move the `MOD` to the correct alpha sorting though, so it should appear after `ESC`.

Code after:
    If the value of a query parameter is None, that parameter will be removed from the URL.
    """

    scheme, netloc, path, query, fragment = urlsplit(url)
    querydict = parse_qs(query)
    querydict.update(params)
    querydict = {key: value for key, value in querydict.items() if value is not None}
    query = urlencode(querydict, doseq=True)
    return urlunsplit((scheme, netloc, path, query, fragment))


def get_keyboard_key_labels_from_request(request):
    """
    Returns an instance of SimpleNamespace based on the user's keyboard layout
    based on the User-Agent header in the request.

    These are intentionally not translated, as they are key labels that are assumed
    to be consistent across all languages.
    """

    user_agent = request.headers.get("User-Agent", "")
    is_mac_os = re.search(r"Mac|iPod|iPhone|iPad", user_agent)

    labels = {
        "ALT": "⌥" if is_mac_os else "Alt",
        "CMD": "⌘" if is_mac_os else "Ctrl",
        "CTRL": "^" if is_mac_os else "Ctrl",
        "DEL": "Delete",
        "ENTER": "Return" if is_mac_os else "Enter",
        "ESC": "Esc",
        "MOD": "⌘" if is_mac_os else "Ctrl",
        "SHIFT": "Shift",
        "TAB": "Tab",
    }

    return SimpleNamespace(**labels)
Repository: wagtail/wagtail
File: setup.py (Python)

Code before:
#!/usr/bin/env python

from wagtail import __version__
from wagtail.utils.setup import assets, check_bdist_egg, sdist

try:
    from setuptools import find_packages, setup
except ImportError:
    from distutils.core import setup


# Hack to prevent "TypeError: 'NoneType' object is not callable" error
# in multiprocessing/util.py _exit_function when setup.py exits
# (see http://www.eby-sarna.com/pipermail/peak/2010-May/003357.html)
try:
    import multiprocessing  # noqa: F401
except ImportError:
    pass


install_requires = [
    "Django>=4.2",
    "django-modelcluster>=6.2.1,<7.0",
    "django-permissionedforms>=0.1,<1.0",
    "django-taggit>=5.0,<7",
    "django-treebeard>=4.5.1,<5.0",
    "djangorestframework>=3.15.1,<4.0",
    "django-filter>=23.3",
    "draftjs_exporter>=2.1.5,<6.0",
    "Pillow>=9.1.0,<12.0.0",
    "beautifulsoup4>=4.8,<5",
    "Willow[heif]>=1.8.0,<2",
    "requests>=2.11.1,<3.0",
    "openpyxl>=3.0.10,<4.0",
    "anyascii>=0.1.5",
    "telepath>=0.3.1,<1",
    "laces>=0.1,<0.2",
    "django-tasks>=0.6.1,<0.7",
]

# Testing dependencies
testing_extras = [

Review comment (suggestion):
Looking at this again, since Python 3 this should simply be

```suggestion
        super().run()
````

I've made the change locally and did e2e testing again, and it still works well. Will push the change as I merge

Code after:
import subprocess

from setuptools import setup
from setuptools.command.sdist import sdist as base_sdist


class sdist(base_sdist):
    def compile_assets(self):
        try:
            subprocess.check_call(["npm", "run", "build"])
        except (OSError, subprocess.CalledProcessError) as e:
            print("Error compiling assets: " + str(e))  # noqa: T201
            raise SystemExit(1)

    def run(self):
        self.compile_assets()
        base_sdist.run(self)


setup(
    name="wagtail",
    cmdclass={"sdist": sdist},
)
Repository: wagtail/wagtail
File: wagtail/users/models.py (Python)

Code before:
        choices=AdminContrastThemes.choices,
        default=AdminContrastThemes.SYSTEM,
        max_length=40,
    )

    class AdminDensityThemes(models.TextChoices):
        DEFAULT = "default", _("Default")
        SNUG = "snug", _("Snug")

    density = models.CharField(
        # Translators: "Density" is the term used to describe the amount of space between elements in the user interface
        verbose_name=_("density"),
        choices=AdminDensityThemes.choices,
        default=AdminDensityThemes.DEFAULT,
        max_length=40,
    )

    @classmethod
    def get_for_user(cls, user):
        return cls.objects.get_or_create(user=user)[0]

    def get_preferred_language(self):
        if self.preferred_language:
            return self.preferred_language
        if get_language() in get_available_admin_languages():
            return get_language()
        return settings.LANGUAGE_CODE

    def get_current_time_zone(self):
        return self.current_time_zone or settings.TIME_ZONE

    def __str__(self):
        return self.user.get_username()

    class Meta:
        verbose_name = _("user profile")
        verbose_name_plural = _("user profiles")


Review comment (suggestion):
suggestion:

```suggestion
        if (language := get_language()) in get_available_admin_languages():
            return language
```

Code after:
        choices=AdminContrastThemes.choices,
        default=AdminContrastThemes.SYSTEM,
        max_length=40,
    )

    class AdminDensityThemes(models.TextChoices):
        DEFAULT = "default", _("Default")
        SNUG = "snug", _("Snug")

    density = models.CharField(
        # Translators: "Density" is the term used to describe the amount of space between elements in the user interface
        verbose_name=_("density"),
        choices=AdminDensityThemes.choices,
        default=AdminDensityThemes.DEFAULT,
        max_length=40,
    )

    @classmethod
    def get_for_user(cls, user):
        return cls.objects.get_or_create(user=user)[0]

    def get_preferred_language(self):
        if self.preferred_language:
            return self.preferred_language
        if (language := get_language()) in dict(get_available_admin_languages()):
            return language
        return settings.LANGUAGE_CODE

    def get_current_time_zone(self):
        return self.current_time_zone or settings.TIME_ZONE

    def __str__(self):
        return self.user.get_username()

    class Meta:
        verbose_name = _("user profile")
        verbose_name_plural = _("user profiles")
Repository: wagtail/wagtail
File: wagtail/images/static_src/wagtailimages/js/focal-point-chooser.js (JavaScript)

Code before:
var jcropapi;

function setupJcrop(image, original, focalPointOriginal, fields) {
  var $chooser = image.closest('div.focal-point-chooser');

  // To avoid an inline style, max-width and max-height are passed as data attributes
  // from the focal-point-chooser class because they are dynamic values
  $chooser.css('max-height', $chooser.data('max-height'));
  $chooser.css('max-width', $chooser.data('max-width'));

  image.Jcrop(
    {
      trueSize: [original.width, original.height],
      bgColor: 'rgb(192, 192, 192)',
      onSelect: function (box) {
        var x = Math.floor((box.x + box.x2) / 2);
        var y = Math.floor((box.y + box.y2) / 2);
        var w = Math.floor(box.w);
        var h = Math.floor(box.h);

        fields.x.val(x);
        fields.y.val(y);
        fields.width.val(w);
        fields.height.val(h);
      },

      onRelease: function () {
        fields.x.val(focalPointOriginal.x);
        fields.y.val(focalPointOriginal.y);
        fields.width.val(focalPointOriginal.width);
        fields.height.val(focalPointOriginal.height);
      },
    },
    function () {

Review comment (refactor):
The original code directly sets the inline styles in the HTML and does nothing with it in the JavaScript. This means we only need to set it once on initial load.

So, I think we can move this to the on load function (the `$(function() {...})` below) so that it's only run once instead of also on resize and "remove" button click. 

Code after:
var jcropapi;

function setupJcrop(image, original, focalPointOriginal, fields) {
  image.Jcrop(
    {
      trueSize: [original.width, original.height],
      bgColor: 'rgb(192, 192, 192)',
      onSelect: function (box) {
        var x = Math.floor((box.x + box.x2) / 2);
        var y = Math.floor((box.y + box.y2) / 2);
        var w = Math.floor(box.w);
        var h = Math.floor(box.h);

        fields.x.val(x);
        fields.y.val(y);
        fields.width.val(w);
        fields.height.val(h);
      },

      onRelease: function () {
        fields.x.val(focalPointOriginal.x);
        fields.y.val(focalPointOriginal.y);
        fields.width.val(focalPointOriginal.width);
        fields.height.val(focalPointOriginal.height);
      },
    },
    function () {
      // eslint-disable-next-line @typescript-eslint/no-this-alias
      jcropapi = this;

      // Set alt="" on the image so its src is not read out loud to screen reader users.
      var $holderImage = $('img', this.ui.holder);
      $holderImage.attr('alt', '');
Repository: wagtail/wagtail
File: client/src/components/Minimap/Minimap.tsx (TypeScript)

Code before:
    setPanelsExpanded(true);
  }, [anchorsContainer, setPanelsExpanded]);

  return (
    <div>
      <CollapseAll
        expanded={panelsExpanded}
        onClick={() => {
          setPanelsExpanded(!panelsExpanded);
          toggleAllPanels(!panelsExpanded);
        }}
        floating
        insideMinimap={expanded}
      />
      <div className={`w-minimap ${expanded ? 'w-minimap--expanded' : ''}`}>
        <div className="w-minimap__header">
          <button
            id="w-minimap-toggle"
            type="button"
            aria-expanded={expanded}
            onClick={onClickToggle}
            className="w-minimap__toggle"
            // Not the most correct label, but matches side panels with similar toggles.
            aria-label={gettext('Toggle side panel')}
            data-controller="w-kbd"
            data-w-kbd-key-value="mod+]"
            data-w-kbd-scope-value="global"
          >
            <Icon name="expand-right" />
          </button>
        </div>
        <ol className="w-minimap__list" ref={listRef}>
          {links.map((link) => (
            <li key={link.href}>
              <MinimapItem
                item={link}
                intersects={intersections[link.href]}
                expanded={expanded}
                onClick={onClickLink}
              />
            </li>
          ))}
        </ol>
        <div className="w-minimap__footer" />
      </div>
    </div>
  );
};

export default Minimap;


Review comment (suggestion):
```suggestion
            data-w-kbd-key-value="alt+]"
```

Code after:
    setPanelsExpanded(true);
  }, [anchorsContainer, setPanelsExpanded]);

  return (
    <div>
      <CollapseAll
        expanded={panelsExpanded}
        onClick={() => {
          setPanelsExpanded(!panelsExpanded);
          toggleAllPanels(!panelsExpanded);
        }}
        floating
        insideMinimap={expanded}
      />
      <div className={`w-minimap ${expanded ? 'w-minimap--expanded' : ''}`}>
        <div className="w-minimap__header">
          <button
            id="w-minimap-toggle"
            type="button"
            aria-expanded={expanded}
            onClick={onClickToggle}
            className="w-minimap__toggle"
            // Not the most correct label, but matches side panels with similar toggles.
            aria-label={gettext('Toggle side panel')}
            data-controller="w-kbd"
            data-w-kbd-key-value="alt+]"
            data-w-kbd-scope-value="global"
          >
            <Icon name="expand-right" />
          </button>
        </div>
        <ol className="w-minimap__list" ref={listRef}>
          {links.map((link) => (
            <li key={link.href}>
              <MinimapItem
                item={link}
                intersects={intersections[link.href]}
                expanded={expanded}
                onClick={onClickLink}
              />
            </li>
          ))}
        </ol>
        <div className="w-minimap__footer" />
      </div>
    </div>
  );
};

export default Minimap;
Repository: wagtail/wagtail
File: wagtail/models/workflows.py (Python)

Code before:
        current_workflow_task_state = self.current_workflow_task_state
        if current_workflow_task_state:
            return current_workflow_task_state.task.specific

    @property
    def status_string(self):
        if not self.live:
            if self.expired:
                return _("expired")
            elif self.approved_schedule:
                return _("scheduled")
            elif self.workflow_in_progress:
                return _("in moderation")
            else:
                return _("draft")
        else:
            if self.approved_schedule:
                return _("live + scheduled")
            elif self.workflow_in_progress:
                return _("live + in moderation")
            elif self.has_unpublished_changes:
                return _("live + draft")
            else:
                return _("live")

    def get_lock(self) -> Union[BaseLock, WorkflowLock, None]:
        # Standard locking should take precedence over workflow locking
        # because it's possible for both to be used at the same time
        if lock := super().get_lock():
            return lock

        if current_workflow_task := self.current_workflow_task:
            lock_class = current_workflow_task.lock_class
            return lock_class(self, current_workflow_task)

        return None


Review comment (suggestion):
```suggestion
    def get_lock(self):
```

The `get_lock` annotation in `LockableMixin` doesn't have annotation yet, so I'm not sure about adding it only here. Also, we've dropped Python 3.9 support, so we can use the `|` union syntax if we want to add type hints.

Code after:
            return current_workflow_task_state.task.specific

    @property
    def status_string(self):
        if not self.live:
            if self.expired:
                return _("expired")
            elif self.approved_schedule:
                return _("scheduled")
            elif self.workflow_in_progress:
                return _("in moderation")
            else:
                return _("draft")
        else:
            if self.approved_schedule:
                return _("live + scheduled")
            elif self.workflow_in_progress:
                return _("live + in moderation")
            elif self.has_unpublished_changes:
                return _("live + draft")
            else:
                return _("live")

    def get_lock(self):
        # Standard locking should take precedence over workflow locking
        # because it's possible for both to be used at the same time
        if lock := super().get_lock():
            return lock

        if current_workflow_task := self.current_workflow_task:
            lock_class = current_workflow_task.lock_class
            return lock_class(self, current_workflow_task)

        return None