ronantakizawa/codereview-bench huggingface dataset:
Repository: wagtail/wagtail
File: wagtail/test/testapp/models.py (Python)

Code before:

    edit_handler = TabbedInterface(
        [
            ObjectList(advert_panels, heading="Advert"),
            ObjectList(
                other_panels, heading="Other", help_text="Other panels help text"
            ),
        ],
        help_text="Top-level help text",
    )

    def __str__(self):
        return self.text

    class Meta:
        ordering = ("text",)


register_snippet(AdvertWithTabbedInterface)


class AdvertManager(models.Manager):
    pass


class AdvertWithCustomManager(models.Model):
    url = models.URLField(null=True, blank=True)
    text = models.CharField(max_length=255)

    adverts = AdvertManager()

    panels = [
        FieldPanel("url"),
        FieldPanel("text"),
    ]

    def __str__(self):
        return self.text


register_snippet(AdvertWithCustomManager)


# Models with RevisionMixin
class RevisableModel(RevisionMixin, models.Model):
    text = models.TextField()


class RevisableChildModel(RevisableModel):
    secret_text = models.TextField(blank=True, default="")


Review comment (suggestion):
Thanks, but we don't need a new model for this, let's just reuse an existing one. Adding a new model would add additional overhead as that's yet another migration to run and another table to create on the database.

Code after:

    edit_handler = TabbedInterface(
        [
            ObjectList(advert_panels, heading="Advert"),
            ObjectList(
                other_panels, heading="Other", help_text="Other panels help text"
            ),
        ],
        help_text="Top-level help text",
    )

    def __str__(self):
        return self.text

    class Meta:
        ordering = ("text",)


register_snippet(AdvertWithTabbedInterface)


class CustomManager(models.Manager):
    pass


class ModelWithCustomManager(models.Model):
    instances = CustomManager()


register_snippet(ModelWithCustomManager)


# Models with RevisionMixin
class RevisableModel(RevisionMixin, models.Model):
    text = models.TextField()


class RevisableChildModel(RevisableModel):
    secret_text = models.TextField(blank=True, default="")

    # The edit_handler is defined on the viewset


class RevisableGrandChildModel(RevisableChildModel):
    pass


# Models with DraftStateMixin
class DraftStateModel(DraftStateMixin, LockableMixin, RevisionMixin, models.Model):
    text = models.TextField()
Repository: wagtail/wagtail
File: client/src/entrypoints/admin/telepath/widgets.js (JavaScript)

Code before:
import { gettext } from '../../../utils/gettext';

class BoundWidget {
  constructor(
    element,
    name,
    idForLabel,
    initialState,
    parentCapabilities,
    options,
  ) {
    var selector = ':is(input,select,textarea,button)[name="' + name + '"]';
    // find, including element itself
    this.input = element.matches(selector)
      ? element
      : element.querySelector(selector);
    this.idForLabel = idForLabel;
    this.setState(initialState);
    this.parentCapabilities = parentCapabilities || new Map();
    this.options = options;
  }

  getValue() {
    return this.input.value;
  }

  getState() {
    return this.input.value;
  }

  setState(state) {
    this.input.value = state;
  }

  getTextLabel(opts) {
    const val = this.getValue();
    if (typeof val !== 'string') return null;

Review comment (suggestion):
Maybe use template literals? Also, I notice that we're changing `var`s to `const` further down, so

```suggestion
    const selector = `:is(input,select,textarea,button)[name="${name}"]`;
```

(there are other similar `+ name +` occurrences below)

Code after:
import { gettext } from '../../../utils/gettext';
import { runInlineScripts } from '../../../utils/runInlineScripts';

class BoundWidget {
  constructor(
    element,
    name,
    idForLabel,
    initialState,
    parentCapabilities,
    options,
  ) {
    const selector = `:is(input,select,textarea,button)[name="${name}"]`;
    // find, including element itself
    this.input = element.matches(selector)
      ? element
      : element.querySelector(selector);
    this.idForLabel = idForLabel;
    this.setState(initialState);
    this.parentCapabilities = parentCapabilities || new Map();
    this.options = options;
  }

  getValue() {
    return this.input.value;
  }

  getState() {
    return this.input.value;
  }

  setState(state) {
    this.input.value = state;
  }

  getTextLabel(opts) {
    const val = this.getValue();
Repository: wagtail/wagtail
File: client/src/entrypoints/admin/telepath/widgets.js (JavaScript)

Code before:
}

class Widget {
  constructor(html, idPattern) {
    this.html = html;
    this.idPattern = idPattern;
  }

  boundWidgetClass = BoundWidget;

  render(
    placeholder,
    name,
    id,
    initialState,
    parentCapabilities,
    options = {},
  ) {
    const html = this.html.replace(/__NAME__/g, name).replace(/__ID__/g, id);
    const idForLabel = this.idPattern.replace(/__ID__/g, id);

    /* write the HTML into a temp container to parse it into an element */
    const tempContainer = document.createElement('div');
    tempContainer.innerHTML = html.trim();
    const dom = tempContainer.firstChild;

    /* replace the placeholder with the new element */
    placeholder.replaceWith(dom);

    /* execute any scripts in the new element */
    dom.querySelectorAll('script').forEach((script) => {
      const newScript = document.createElement('script');
      newScript.text = script.text;
      script.replaceWith(newScript);
    });

    // Add any extra attributes we received to the HTML of the widget
    if (typeof options?.attributes === 'object') {
      Object.entries(options.attributes).forEach(([key, value]) => {
        dom.setAttribute(key, value);
      });
    }

    // eslint-disable-next-line new-cap
    return new this.boundWidgetClass(
      dom,
      name,
      idForLabel,
      initialState,
      parentCapabilities,
      options,

Review comment (suggestion):
I'm not familiar with how this code is used, but I just wanted to flag that this means the html string can only consist of one node in the top-level, and any other top-level nodes will be ignored. Probably OK for the widget's use case, but if multiple top-level nodes were desired, then we'd need something similar to what I did in 2003e14a957d783e1a308765082b8d269c08b16c

Code after:
  }
}

class Widget {
  constructor(html, idPattern) {
    this.html = html;
    this.idPattern = idPattern;
  }

  boundWidgetClass = BoundWidget;

  render(
    placeholder,
    name,
    id,
    initialState,
    parentCapabilities,
    options = {},
  ) {
    const html = this.html.replace(/__NAME__/g, name).replace(/__ID__/g, id);
    const idForLabel = this.idPattern.replace(/__ID__/g, id);

    /* write the HTML into a temp container to parse it into an element */
    const tempContainer = document.createElement('div');
    tempContainer.innerHTML = html.trim();
    const dom = tempContainer.firstChild;

    /* replace the placeholder with the new element */
    placeholder.replaceWith(dom);

    /* execute any scripts in the new element */
    runInlineScripts(dom);

    // Add any extra attributes we received to the HTML of the widget
    if (typeof options?.attributes === 'object') {
      Object.entries(options.attributes).forEach(([key, value]) => {
        dom.setAttribute(key, value);
      });
    }

    // eslint-disable-next-line new-cap
    return new this.boundWidgetClass(
      dom,
      name,
      idForLabel,
      initialState,
      parentCapabilities,
      options,
    );
  }
}
Repository: wagtail/wagtail
File: wagtail/wagtail_hooks.py (Python)

Code before:
                    "task": log_entry.data["workflow"]["task"]["title"],
                }
            except (KeyError, TypeError):
                return _("Workflow task resubmitted. Workflow resumed")

    @actions.register_action("wagtail.workflow.cancel")
    class CancelWorkflowActionFormatter(WorkflowLogFormatter):
        label = _("Workflow: cancel")

        def format_message(self, log_entry):
            try:
                return _("Cancelled '%(workflow)s' at '%(task)s'") % {
                    "workflow": log_entry.data["workflow"]["title"],
                    "task": log_entry.data["workflow"]["task"]["title"],
                }
            except (KeyError, TypeError):
                return _("Workflow cancelled")


@hooks.register("before_serve_page", order=0)
def serve_options_response(page: Page, request: HttpRequest, *args, **kwargs):
    """Handle responding to requests for the OPTIONS HTTP verb."""
    if request.method == "OPTIONS" and "options" in page.allowed_http_methods_lower:
        response = HttpResponse()
        response.headers["Allow"] = ", ".join(sorted(page.allowed_http_methods_upper))
        response.headers["Content-Length"] = "0"
        return response
    return None


Review comment (suggestion):
Suggestion: Let Django / the app server deal with setting the content length. 
```suggestion
```

Code after:
        label = _("Workflow: resume task")

        def format_message(self, log_entry):
            try:
                return _("Resubmitted '%(task)s'. Workflow resumed") % {
                    "task": log_entry.data["workflow"]["task"]["title"],
                }
            except (KeyError, TypeError):
                return _("Workflow task resubmitted. Workflow resumed")

    @actions.register_action("wagtail.workflow.cancel")
    class CancelWorkflowActionFormatter(WorkflowLogFormatter):
        label = _("Workflow: cancel")

        def format_message(self, log_entry):
            try:
                return _("Cancelled '%(workflow)s' at '%(task)s'") % {
                    "workflow": log_entry.data["workflow"]["title"],
                    "task": log_entry.data["workflow"]["task"]["title"],
                }
            except (KeyError, TypeError):
                return _("Workflow cancelled")


@hooks.register("before_serve_page", order=0)
def check_request_method(page: Page, request: "HttpRequest", *args, **kwargs):
    """
    Before serving, check the request method is permitted by the page,
    and use the page object's :meth:``wagtail.models.Page.handle_options_request``
    method to generate a response if the OPTIONS HTTP verb is used.
    """
    check_response = page.check_request_method(request, *args, **kwargs)
    if check_response is not None:
        return check_response
    if request.method == HTTPMethod.OPTIONS.value:
        return page.handle_options_request(request, *args, **kwargs)
    return None
Repository: wagtail/wagtail
File: client/src/utils/forceFocus.ts (TypeScript)

Code before:
import { debounce } from './debounce';

/**
 * Trigger a focus on a potentially non-focusable element, awaiting
 * for any delays in DOM creation. Smoothly scroll to the element
 * if it is not already in view, then trigger a DOM focus.
 *
 * Once the target element loses focus, the `tabindex` attribute,
 * if it was added, remove the attribute.
 *
 * @example
 * ```ts
 * forceFocus(document.getElementById('my-element') as HTMLElement);
 * ```
 */
export const forceFocus = debounce(
  (element: Element | null, { preventScroll = false }: FocusOptions = {}) => {
    if (!element) return;

    if (!element.hasAttribute('tabindex')) {
      const handleBlur = () => {
        element.removeAttribute('tabindex');
        element.removeEventListener('blur', handleBlur);
        element.removeEventListener('focusout', handleBlur);
      };

      element.setAttribute('tabindex', '-1');
      element.addEventListener('blur', handleBlur);
      element.addEventListener('focusout', handleBlur);
    }

    if (!preventScroll) {
      element.scrollIntoView({ behavior: 'smooth', block: 'center' });
    }

    if (element instanceof HTMLElement) {
      element.focus({ preventScroll });
    }
  },
  50,
);


Review comment (suggestion):
Is there any particular reason why this accepts `Element` rather than `HTMLElement`?

Also, I saw that in `FocusController` we still check whether the element is `null` or not (i.e. `if (target) forceFocus(target)`). If the expectation is that the caller is responsible for the validity of the element, then the util function can just expect the element param to be a non-nullable `HTMLElement`.

I know that it's more of a defensive technique, but I think TypeScript would help prevent passing null. And in the case of non-TS code or accidental mistake at runtime, we'd want to know of such errors instead of silencing them.

Code after:
import { debounce } from './debounce';

/**
 * Trigger a focus on a potentially non-focusable element, awaiting
 * for any delays in DOM creation. Smoothly scroll to the element
 * if it is not already in view, then trigger a DOM focus.
 *
 * Once the target element loses focus, the `tabindex` attribute,
 * if it was added, remove the attribute.
 *
 * @example
 * ```ts
 * forceFocus(document.getElementById('my-element') as HTMLElement);
 * ```
 */
export const forceFocus = debounce(
  (
    element: HTMLElement | SVGElement,
    { preventScroll = false }: FocusOptions = {},
  ) => {
    if (!element.hasAttribute('tabindex')) {
      const handleBlur = () => {
        element.removeAttribute('tabindex');
        element.removeEventListener('blur', handleBlur);
        element.removeEventListener('focusout', handleBlur);
      };

      element.setAttribute('tabindex', '-1');
      element.addEventListener('blur', handleBlur);
      element.addEventListener('focusout', handleBlur);
    }

    if (!preventScroll) {
      element.scrollIntoView({ behavior: 'smooth', block: 'center' });
    }

    element.focus({ preventScroll });
  },
  50,
);
Repository: wagtail/wagtail
File: client/src/utils/forceFocus.ts (TypeScript)

Code before:
 * forceFocus(document.getElementById('my-element') as HTMLElement);
 * ```
 */
export const forceFocus = debounce(
  (element: Element | null, { preventScroll = false }: FocusOptions = {}) => {
    if (!element) return;

    if (!element.hasAttribute('tabindex')) {
      const handleBlur = () => {
        element.removeAttribute('tabindex');
        element.removeEventListener('blur', handleBlur);
        element.removeEventListener('focusout', handleBlur);
      };

      element.setAttribute('tabindex', '-1');
      element.addEventListener('blur', handleBlur);
      element.addEventListener('focusout', handleBlur);
    }

    if (!preventScroll) {
      element.scrollIntoView({ behavior: 'smooth', block: 'center' });
    }

    if (element instanceof HTMLElement) {
      element.focus({ preventScroll });
    }
  },
  50,
);


Review comment (refactor):
The reason why I raised the comment about the typing of the `element` parameter is because of this `instanceof` check. Doing a runtime `HTMLElement` check to conditionally call the `focus()` method seems strange for a function that's supposed to force focus on (what's supposed to be) an HTML element.

However, I can see a different design where we intend the util function to be used with any `querySelector`/`getElementById`/etc. result directly (without further checks) – which may be `Element` (that's not `HTMLElement`) or `null`, and we only want the operation to be done if it is indeed an `HTMLElement`. If this is the design we want, then we can just move this check to the early return at the start of the function, i.e.

```ts
if (!(element instanceof HTMLElement)) return;
```

and keep the TS signature as-is.

Code after:
 * forceFocus(document.getElementById('my-element') as HTMLElement);
 * ```
 */
export const forceFocus = debounce(
  (
    element: HTMLElement | SVGElement,
    { preventScroll = false }: FocusOptions = {},
  ) => {
    if (!element.hasAttribute('tabindex')) {
      const handleBlur = () => {
        element.removeAttribute('tabindex');
        element.removeEventListener('blur', handleBlur);
        element.removeEventListener('focusout', handleBlur);
      };

      element.setAttribute('tabindex', '-1');
      element.addEventListener('blur', handleBlur);
      element.addEventListener('focusout', handleBlur);
    }

    if (!preventScroll) {
      element.scrollIntoView({ behavior: 'smooth', block: 'center' });
    }

    element.focus({ preventScroll });
  },
  50,
);
Repository: wagtail/wagtail
File: wagtail/admin/views/tags.py (Python)

Code before:
from django.conf import settings
from django.contrib.contenttypes.models import ContentType
from django.http import Http404, JsonResponse
from taggit.models import Tag, TagBase

WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT = getattr(settings, "WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT", 10)

def autocomplete(request, app_name=None, model_name=None):
    if app_name and model_name:
        try:
            content_type = ContentType.objects.get_by_natural_key(app_name, model_name)
        except ContentType.DoesNotExist:
            raise Http404

        tag_model = content_type.model_class()
        if not issubclass(tag_model, TagBase):
            raise Http404

    else:
        tag_model = Tag

    term = request.GET.get("term", None)
    if term:
        tags = (
            tag_model.objects.filter(name__istartswith=term)
            .order_by("name")
            .values_list("name", flat=True)[:WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT]
        )
    else:
        tags = tag_model.objects.none()


Review comment (suggestion):
@aayushman-singh  for now, as per discussion on https://github.com/wagtail/wagtail/issues/12588 - let's keep this away from being a setting.

However, it may be nice to have this 'constant' pulled out to the top of this file anyway, just without reading from settings.

```suggestion
WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT = 10
```

This gives an avenue for an unofficial path (monkey patch) to override this if developers get really stuck.

Code after:
from django.contrib.contenttypes.models import ContentType
from django.http import Http404, JsonResponse
from taggit.models import Tag, TagBase

TAGS_AUTOCOMPLETE_LIMIT = 10


def autocomplete(request, app_name=None, model_name=None):
    if app_name and model_name:
        try:
            content_type = ContentType.objects.get_by_natural_key(app_name, model_name)
        except ContentType.DoesNotExist:
            raise Http404

        tag_model = content_type.model_class()
        if not issubclass(tag_model, TagBase):
            raise Http404

    else:
        tag_model = Tag

    term = request.GET.get("term", None)
    if term:
        tags = (
            tag_model.objects.filter(name__istartswith=term)
            .order_by("name")
            .values_list("name", flat=True)[:TAGS_AUTOCOMPLETE_LIMIT]
        )
    else:
        tags = tag_model.objects.none()
Repository: wagtail/wagtail
File: wagtail/admin/views/tags.py (Python)

Code before:
from django.conf import settings
from django.contrib.contenttypes.models import ContentType
from django.http import Http404, JsonResponse
from taggit.models import Tag, TagBase

WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT = 10

def autocomplete(request, app_name=None, model_name=None):
    if app_name and model_name:
        try:
            content_type = ContentType.objects.get_by_natural_key(app_name, model_name)
        except ContentType.DoesNotExist:
            raise Http404

        tag_model = content_type.model_class()
        if not issubclass(tag_model, TagBase):
            raise Http404

    else:
        tag_model = Tag

    term = request.GET.get("term", None)
    if term:
        tags = (
            tag_model.objects.filter(name__istartswith=term)
            .order_by("name")
            .values_list("name", flat=True)[:WAGTAIL_TAGS_AUTOCOMPLETE_LIMIT]
        )
    else:
        tags = tag_model.objects.none()


Review comment (refactor):
Do you mind updating this to 

```suggestion
TAGS_AUTOCOMPLETE_LIMIT = 10
```

Without the WAGTAIL_ prefix.

If we do add a setting usage in the future we can rename accordingly.

For now it's just an abstracted constant.

Also a reminder to remove the `settings` import as it's no longer used.

Code after:
from django.contrib.contenttypes.models import ContentType
from django.http import Http404, JsonResponse
from taggit.models import Tag, TagBase

TAGS_AUTOCOMPLETE_LIMIT = 10


def autocomplete(request, app_name=None, model_name=None):
    if app_name and model_name:
        try:
            content_type = ContentType.objects.get_by_natural_key(app_name, model_name)
        except ContentType.DoesNotExist:
            raise Http404

        tag_model = content_type.model_class()
        if not issubclass(tag_model, TagBase):
            raise Http404

    else:
        tag_model = Tag

    term = request.GET.get("term", None)
    if term:
        tags = (
            tag_model.objects.filter(name__istartswith=term)
            .order_by("name")
            .values_list("name", flat=True)[:TAGS_AUTOCOMPLETE_LIMIT]
        )
    else:
        tags = tag_model.objects.none()
Repository: wagtail/wagtail
File: wagtail/users/utils.py (Python)

Code before:
from django.utils.translation import gettext_lazy as _

from wagtail.compat import AUTH_USER_APP_LABEL, AUTH_USER_MODEL_NAME
from wagtail.coreutils import safe_md5

delete_user_perm = "{}.delete_{}".format(
    AUTH_USER_APP_LABEL, AUTH_USER_MODEL_NAME.lower()
)


def user_can_delete_user(current_user, user_to_delete):
    if not current_user.has_perm(delete_user_perm):
        return False

    if current_user == user_to_delete:
        # users may not delete themselves
        return False

    if user_to_delete.is_superuser and not current_user.is_superuser:
        # ordinary users may not delete superusers
        return False

    return True


def get_gravatar_url(email, size=50, default_params={}):
    DEFAULT_IMAGE = "mp"
    DEFAULT_GRAVATAR_PROVIDER_URL = "www.gravatar.com/avatar"

    params = {
        "s": int(size)
        * 2,  # requested at retina size by default and scaled down at point of use with css
        "d": DEFAULT_IMAGE,
    }

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", DEFAULT_GRAVATAR_PROVIDER_URL
    )

    if (not email) or (gravatar_provider_url is None):
        return None

    parsed_url = urlparse(gravatar_provider_url.rstrip("/"))

    if parsed_url.query:
        params.update(
            **{key: value[-1] for key, value in parse_qs(parsed_url.query).items()},
        )
    params.update(**default_params)

    email_hash = safe_md5(

Review comment (suggestion):
```suggestion
def get_gravatar_url(email, size=50, default_params={"d": "mp"}):
```

Let's put the default `mp` into the default_params here.

Code after:
from django.utils.translation import gettext_lazy as _

from wagtail.compat import AUTH_USER_APP_LABEL, AUTH_USER_MODEL_NAME
from wagtail.coreutils import safe_md5

delete_user_perm = "{}.delete_{}".format(
    AUTH_USER_APP_LABEL, AUTH_USER_MODEL_NAME.lower()
)


def user_can_delete_user(current_user, user_to_delete):
    if not current_user.has_perm(delete_user_perm):
        return False

    if current_user == user_to_delete:
        # users may not delete themselves
        return False

    if user_to_delete.is_superuser and not current_user.is_superuser:
        # ordinary users may not delete superusers
        return False

    return True


def get_gravatar_url(email, size=50, default_params={"d": "mp"}):
    """
    See https://gravatar.com/site/implement/images/ for Gravatar image options.

    Example usage:

    .. code-block:: python

        # Basic usage
        gravatar_url = get_gravatar_url('user@example.com')

        # Customize size and default image
        gravatar_url = get_gravatar_url(
            'user@example.com',
            size=100,
            default_params={'d': 'robohash', 'f': 'y'}
        )

    Note:
        If any parameter in ``default_params`` also exists in the provider URL,
        it will be overridden by the provider URL's query parameter.
    """

    gravatar_provider_url = getattr(
        settings, "WAGTAIL_GRAVATAR_PROVIDER_URL", "//www.gravatar.com/avatar"
    )