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

Add more flexibility in URLs for MentionableMixin models #28

Closed
philgyford opened this issue Mar 27, 2022 · 3 comments · Fixed by #29
Closed

Add more flexibility in URLs for MentionableMixin models #28

philgyford opened this issue Mar 27, 2022 · 3 comments · Fixed by #29

Comments

@philgyford
Copy link

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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.
3. Seems wrong - not sure if it's a bug or if I need to do something further in my resolve_from_url_kwargs() method?
4. 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.
5. 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants