beatonma / django-wm

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

Add an option to work without Celery? #17

Closed philgyford closed 1 year ago

philgyford commented 2 years ago

I want to add webmentions to my own website and I really like the look of django-wm – there are so many nice and useful touches. But I don't currently use Celery on it and I'm unlikely to get it set up just to enable webmentions.

Would you be open to me adding an option to work without Celery? e.g. Add two custom management commands to process any outstanding incoming and outgoing webmentions, which could then be run using cron or similar.

I haven't thought in further detail what would be involved in making this work – maybe it would require either adding extra field(s) to the models to indicate whether they were waiting to be processed, or adding another model to store this queue...

Anyway, if you're open to the idea I'd be happy to give it a go, and if you have any better ideas about how to approach it, that would be appreciated.

beatonma commented 2 years ago

Hey, sorry for the late response. Good idea! You are absolutely welcome to work on this and I'd be happy to accept any pull requests that make it more useful/usable.

I started thinking about how I would approach this yesterday and ended up just starting it - https://github.com/beatonma/django-wm/compare/without-celery - basically using the approach you suggested.

The commit is messy but basically:

I ran out of time so this is completely untested and needs some tidying up but it should be close to working..? Feel free to use this as a starting point if there is anything else you want to add or fix.

philgyford commented 2 years ago

Oh this looks great, thank you! Probably nicer than whatever I'd have attempted. I'll try and give it a whirl in the next few days and see if I can think of anything else to do with it. Thanks again.

philgyford commented 2 years ago

Sorry I didn't get a chance to look at this sooner - Christmas, New Year, etc got in the way.

I've only had a quick go so far, but some initial questions:

  1. Is there a reason why the app doesn't have any migrations? I know I'd have to create a migration for my own app after adding MentionableMixin to a model, but it seems very unusual for there to be no migrations for the non-mixin models.
  2. There's a typo in tasks/scheduling.py on line 14 - it should be WEBMENTIONS_USE_CELERY not WEBMENTION_USE_CELERY (no S).
  3. It would be useful to have Admin classes for PendingIncomingWebmention and PendingOutgoingContent so it's possible to see what's waiting to be dealt with. Otherwise it's hard to know if things are working.
  4. I don't think there's anything that prevents duplicate mentions being sent. This can happen (a) if a text links to the same URL more than once, or (b) if an object is saved more than once. I guess this should happen in tasks.outgoing_webmentions.process_outgoing_webmentions()?
beatonma commented 2 years ago

No problem, same deal with festivities here.

1: Not really. At the time of original release I had seen discussion on whether migrations should be included in git and decided it made sense for the consumer to make their own migrations. I have since learnt the error of my ways. Adding them now may complicate things for any existing users but it's probably the right thing to do overall.

2, 3, 4a: Thanks, will fix.

4b: This was intentional so that the mentioned site can know that the source content has changed for moderation purposes. Of course there's nothing stopping you from changing the content quietly anyway, but that's the intent for allowing duplicates. That said, it may be good to add some sort of checks on incoming mentions.

I'm going to work on this a bit this week. I'm also planning to create a testing tool on my main site - I took down the django-wm.dev testing site a while ago as it wasn't being used much, but it would be handy to have that functionality available again.

philgyford commented 2 years ago

Brilliant, thanks! I was hoping to have some time to make the obvious fixes (2, 3) myself but work got in the way. I also haven't worked out how best to use a fork of a third-party app within the local dev version of my site, so it's easier to make changes for a PR.

Yes, you're right about 4b, despite having worked through the Webmention spec some time ago I forgot that it's good to send again in case anything changes. But then, as you say, the onus is on the incoming side to handle any duplicate/updated webmentions appropriately.

beatonma commented 2 years ago

Hey, just want to update you on this - I got a bit sidetracked while making the testing tool I mentioned. Back to this...

Based on recommendations from @GriceTurrble in #19 #20, I have just published a version 1.3.1. It doesn't change anything functionality-wise but just adds a warning for existing users that migrations will soon be included and that this will be a breaking change.

Next step will be to publish version 2.0.0. This will include the migration files, and bump the recommended celery version due to a recent advisory. Both of these are potentially breaking changes so think it makes sense to bundle them together.

Finally, I will publish version 2.1.0 which will include the changes you requested for bypassing celery. Hopefully this will happen in the next day or two - sorry it's taken so long!

philgyford commented 2 years ago

No problem at all! Thank you so much for doing all this!

I did post a hypothetical question to r/django about how one would add migrations to a package if it was already being used by people. @GriceTurrble asked me in chat what the package was, so thanks to him for offering such clear advice too!

❤️

beatonma commented 2 years ago

2.1.0 is now available and the testing tool is now up and running. Updated setup instructions (without celery) start here. Let me know how you get on :)