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

Allow ModelViewSet to be used for models with non-integer primary keys #10900

Merged
merged 7 commits into from
Sep 21, 2023

Conversation

laymonage
Copy link
Member

@laymonage laymonage commented Sep 15, 2023

Similar fixes to #4166 and #7208.

To test:

I don't have a bakerydemo branch for this, but I've been using the testapp. You can configure this in the following:

  • Edit DEBUG in wagtail/test/settings.py to True (would love to change this in a separate PR)
  • Run:
    cd wagtail/test
    DATABASE_NAME=wagtail.db python manage.py migrate
    DATABASE_NAME=wagtail.db python manage.py createcachetable
    DATABASE_NAME=wagtail.db python manage.py loaddata test.json # optional
    DATABASE_NAME=wagtail.db python manage.py loaddata test_specific.json # optional
    DATABASE_NAME=wagtail.db python manage.py createsuperuser
    DATABASE_NAME=wagtail.db python manage.py runserver

The server will run with the testapp configured. You can then use the Feature Complete Toy model to test the URLs. For testing the legacy redirects in ModelViewSet, you need to use a primary key string that's also a valid integer, as the old URL patterns use <int:pk>.

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 new features: Has the documentation been updated accordingly?

Footnotes

  1. Development Testing

@squash-labs
Copy link

squash-labs bot commented Sep 15, 2023

Manage this branch in Squash

Test this branch here: https://laymonagemodelviewset-non-inte-levys.squash.io

Comment on lines +18 to 23
"strid",
models.CharField(
primary_key=True,
default=wagtail.test.testapp.models.random_quotable_pk,
max_length=255,
serialize=False,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this should be in a new migration, I don't think it's worth it because the testapp is pretty much ephemeral and not intended to be long-lived. Properly migrating this would need more than one migration (to add the new field, populate, remove the old one).

We could start with a new model, but I intended this model to be the main model to test all ModelViewSet features (hence "feature complete").

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.

All looks good to me!

In ModelViewSet, this doesn't do anything as the base ViewSet class
returns an empty list.

In SnippetViewSet, this causes the ModelViewSet's urlpatterns to be
carried over, resulting in duplicate URLs that we do not want.
In case people hardcode the URLs instead of using reverse().

Unify the legacy redirects with SnippetViewSet to use the same
_legacy_urlpatterns property.
laymonage added a commit that referenced this pull request Sep 21, 2023
@laymonage laymonage merged commit ce8f655 into wagtail:main Sep 21, 2023
12 of 15 checks passed
gasman added a commit to gasman/wagtail-generic-chooser that referenced this pull request Sep 22, 2023
wagtail/wagtail#10900 changed the default URL pattern for the edit view in ModelAdminViewset to place `/edit/` before the ID.
gasman added a commit to gasman/wagtail-generic-chooser that referenced this pull request Sep 22, 2023
wagtail/wagtail#10900 changed the default URL pattern for the edit view in ModelAdminViewset to place `/edit/` before the ID.
gasman added a commit to wagtail/wagtail-generic-chooser that referenced this pull request Sep 22, 2023
wagtail/wagtail#10900 changed the default URL pattern for the edit view in ModelAdminViewset to place `/edit/` before the ID.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants