beatonma / django-wm

Automatic Webmention functionality for Django models
https://beatonma.org/webmentions_tester/
GNU General Public License v3.0
13 stars 2 forks source link

Add more flexibility in URLs for MentionableMixin models #28

Closed philgyford closed 2 years ago

philgyford commented 2 years ago

A difficulty I've found integrating this into my existing site is with the requirement for the URL pattern for a MentionableMixin model to just have a slug.

I've added MentionableMixin to my Post model, but the URL pattern for its get_absolute_url() / detail view is like:

"<slug:blog_slug>/<int:year>/<int:month>/<int:day>/<slug:post_slug>/"

I could change post_slug to be just slug but the Post.slug field has a unique_for_date constraint on it, and there are many Posts with identical slugs, but with different dates. So the get_model_for_url_path() wouldn't be able to find a Post solely by using the slug field.

My first thought at a solution to this would be to add an optional setting to replace the get_model_for_url_path() function, something like this (as a default):

WEBMENTIONS_GET_MODEL_FUNCTION = "mentions.resolution.get_model_for_url_path"

I'm not sure that's the best name, or that it's the best idea, but still.

It would allow the replacement of that function with another that would receive a URL and optional ResolverMatch and either return the found model or else raise one of BadConfig or TargetDoesNotExist exceptions. It would give flexibility over exactly how that model should be found. Is that enough flexibility? Is it a bit too complicated to describe how to replace the function? Will this all end in tears?

As ever, I'm open to other, better ideas.

philgyford commented 2 years ago

Wow, you are on fire :) 🔥 🔥

Having it as a method on the mixin is a much better idea than mine too, very good.

Thank you. I shall give this a whirl tomorrow.

beatonma commented 2 years ago

Thanks for the suggestion - it was silly to have it require a unique slug in the first place, this makes it much more flexible.

I had a look at your repo and used a simplified version of your models and urlpatterns to test against so hopefully it's good to go!

philgyford commented 2 years ago

Just wanted to let you know that I've got the basics working and have it live on my site now, quietly! Thank you so much for your assistance.

I just need to work out how best to make it visible to people on my site now which is my own problem :)

One odd thing I noticed with my /webmentions/ endpoint, and I'm not sure if it's intentional...

  1. Using a valid blog post target URL returns a 200 and no error message
  2. Using an invalid URL returns a 404 and the "Target not found" message
  3. Using a URL that's a valid blog post URL format, but doesn't match a post object returns a 200 and no error message
  4. Using the valid target URL but without the trailing slash returns a 404 and the "Target not found" message
  5. Using a URL that's any other valid page, but not a blog post, returns a 200 and no error message

1 & 2 seem correct.

  1. Seems wrong - not sure if it's a bug or if I need to do something further in my resolve_from_url_kwargs() method?
  2. Is maybe technically correct, but I've seen bits of the django-wm code that tries both with and without trailing slashes, so I was surprised.
  3. I could see a case for being correct - the page does exist! I assume it's not within the scope of the webmention endpoint to say whether it accepts mentions or not?