Upgrade to Django 4

[tvanesse]

Hi there,

It seems this app is not compatible with Django 4+ and I will need to implement the same feature in a Django 4 project anyway. So instead of doing the job twice, I thought I might work on making this package compatible with Django 4 directly.

I am happy to provide a PR with all the required modifications, but is this repo still alive and will my work be merged eventually?
Also a bit of guidance beforehand would be great, if possible ;-)

Waiting for your confirmation that the repo is still being maintained and welcoming PRs, as well as some guidance on how to proceed with the migration to Django 4+.

Cheers.

[OndrejSodek]

Hello,

Yes we are happy to accept a PR to improve compatibility, thank you for the initiative :-)

I've very quickly setup a basic django project and it seems the main problem is that Django 4.0 removed ugettext_lazy, which then results in an ImportError when the app is installed to the django project. This can just be replaced by gettext_lazy.

Did you encounter a different issue?

I also can't install the package on python 3.12, but that's a different issue. Python 3.11 works fine.

We'd welcome if you want to test it a bit more and send a PR to fix the issue(s), I'm happy to help with any issues that come up.

Thanks

[tvanesse]

Hi there,

Yes I've already swtiched from ugettext_lazy to gettext_lazy but I still can't make it work with my current project. It seems the page_lock_template_data and page_lock_api_interval variables don't get populated in the ModelAdmin inheriting from PageLockAdminMixin and, as a result, the JS part that's supposed to render the countdown is raising an exception.

I believe there might be some problems with other third party libraries I'm using in this project. I am guessing some things could clash with django-admin-interface but I still need to investigate a bit further.

[OndrejSodek]

I see, when I tested the full functionality, I encountered the problem as well. It's because the package uses $ as an alias to jQuery, but Django no longer uses $ as an alias to jQuery.

Django 4 also removed django.conf.urls.url in favor of django.urls.re_path.

I've fixed these 2 issues and the ugettext_lazy and pushed it to a branch django-4.
With those fixes it was working for me, but it's possible I missed something.

One more thing, the README doesn't say it, but you also need to add this to your urls so that the JS functionality can call the endpoints. That definitely needs to be added to the README.
path('page_lock/', include('admin_page_lock.urls')),

Let me know if this solves your issue and if you find anything else that needs improving.
Feel free to create a PR, alternatively I will polish it and push it after we can confirm it works fine.

For the record, I tested it with this ModelAdmin

class CategoryAdmin(PageLockAdminMixin, admin.ModelAdmin):
    lock_change_view = True
    lock_changelist_view = True

and this template

{% extends 'admin/change_form.html' %}

{% load page_lock_bar %}

{% block content %}
    {% page_lock_bar_plain %}
    {{ block.super }}
{% endblock %}

[tvanesse]

Hello,

I was on the same track (moved from url() to path) and found that, although indeed not documented, we have to import the package urls into the project to have the API endpoints exposed. So far so good.

I still have one problem with the change_view method not being executed on the inheriting ModelAdmin. I am working with this ModelAdmin:

@admin.register(Protocol)
class ProtocolAdmin(ExtraButtonsMixin, PageLockAdminMixin, admin.ModelAdmin):
    ...

and ExtraButtonsMixin does not override change_view so it should not have any impact on that matter.
EDIT: I just double-checked by removing the inheritance from ExtraButtonsMixin altogether and the problem remains. So that is definitely out of the way.

If I let that ModelAdmin as is, the change_view from PageLockAdminMixin is not executed and, consequently, I don't get the extra_context required to render the change_form.html together with the JS portion.

If, on the other hand, I force-override change_view in the ModelAdmin like this:

@admin.register(Protocol)
class ProtocolAdmin(ExtraButtonsMixin, PageLockAdminMixin, admin.ModelAdmin):
    ....
    def change_view(self, req, object_id, form_url="", extra_context=None):
        print("change_view gets called")
        ans = super().change_view(req, object_id, form_url, extra_context)

        context = ans.context_data
        print(context.get("page_lock_template_data"))  # <--- prints out "None"
        print(context.get("page_lock_api_interval"))   # <--- prints out "None"

        connection_data = self._open_page_connection_data(req)
        updated_context = self._add_extra_content(req, connection_data)
        ans.context_data.update(updated_context)

        context = ans.context_data
        print(context.get("page_lock_template_data")) # <--- prints out the expected data
        print(context.get("page_lock_api_interval"))  # <--- prints out the expected data
        return ans

that solves the problem and the lock-page feature works as intended.

I don't quite understand why change_view is not being properly inherited. But that's what I'm trying to figure out right now.

[tvanesse]

Wait, I just realised lock_change_view is simply not True by default and you have to set it explicitly in the ModelAdmin implementation (as you did) :-) Now it's working as intended.

I believe it would make sense to have lock_change_view = True by default. Because if you inherit from PageLockAdminMixin, it's probably because you want to use that feature in the first place. But that's just an opinion.

I was also able to make it work from your branch django-4 so I guess all there is left is to update the README with

• the instructions to import the URLs
• setting lock_change_view and/or lock_changelist_view to True in the inheriting views/ModelAdmin

Another improvement I see would be to embed all the package settings variables into a dictionary (something like PAGE_LOCK_SETTINGS = {...} to avoid name clashes with other settings. I think names like API_INTERVAL are quite broad and could lead to some problems down the road. But again, that's just an opinion and the package works like a charm as it is 😉

Do you want me to fork your django-4 branch, update the README and file a PR? Or do you want to do it yourself?

[OndrejSodek]

Glad to see you got it working :)

And yeah, I definitely agree with your points.
The settings should probably be separate prefixed variables, as is standard for Django. In any case the current state is not great.

Regarding the PR, these changes also mean that we no longer support python 2.7 and only support Django 2.0+, so we have to increase the major version as this is a breaking change.

Also in Django 3.2 there's the DEFAULT_AUTO_FIELD settings, the package needs to specify which field to use for the Auto primary key, otherwise makemigrations will generate a migration for this package.

So there need to be few more changes, then we can release a new version of the package properly.
I'll look into it shortly.

Regarding the missing documentation, I'll edit the README and merge it. In any case, thanks for reviving this package 👍

It's always great to see people who are interested in improving the package :)
If you'd like to improve the package more, change the default to lock_change_view = True, change the settings variable names, we'd welcome such contributions.