Skip to content

Extension ots newsletter signup - #4149

Closed
frankduncan wants to merge 7 commits into
mainfrom
extension-ots-newsletter-signup
Closed

Extension ots newsletter signup#4149
frankduncan wants to merge 7 commits into
mainfrom
extension-ots-newsletter-signup

Conversation

@frankduncan

Copy link
Copy Markdown
Contributor

This is the first of the OTS extensions! This one adds a field to the User object so that users can specify that they'd like to sign up for a newsletter. This has been requested by multiple clients, but should not be added to the base User object since it's not a requirement for all clients! It also provides an example of how to modify base hypha models to add extra information.

The list of commits is broken up by functionality, with 09243ad, 00bc1fc, 00ba65d, f952639, f970bf1 being small commits required to make the overall purpose of the extension to work. The final commit adds the bulk of the extension.

Some notes about how were going about this:

  • Tests are run if the extension is imported. This may mean we want to import this (and all further extensions) into test.py to ensure they always get run, even if a deployment does not want them included
  • The migrations live outside of the main codebase, and therefore will never cause conflicts
  • The final commit does add the extension configuration to extensions.py, but if this is accepted, we'll probably want to disable that so that this extension is not enabled by default. I defer to you guys on that decision.
  • We have not yet created documentation for this new concept. If you guys approve in general, and accept the PR either in part (the commits we need to create the extension) or in whole, we should figure out a place that extension documentation should live.

This creates a place to put extensions django applications so that
base.py doesn't run into extra changes for people who want extensions.

Extension based configuration will also go here.
This is pulled from https://github.com/nitely/django-hooks with
everything except the template hooks removed.
This migration loads up Users to update their groups.  However, if the
User class has been changed in some way, and the migration runs from a
clean database, it will fail with the error that you cannot select the
new field from the database (as it hasn't been created!)

This change makes it so that only the groups will be loaded, which is
the only part of the User model that is needed for the migration.
This gives any extension the ability to add css and javascript to every
page in hypha.
Allows extensions that want to add new fields to users to have those
fields present in the wagtail admin.
@frankduncan

Copy link
Copy Markdown
Contributor Author

Small modification to the above. 00bc1fc is a bigger commit that adds in the code from a defunct django library to make it so hooks can be used within templates

@frankduncan

Copy link
Copy Markdown
Contributor Author

If you guys like the direction here, I'll figure out to make it so tests do what we want while ensuring that makemigrations doesn't error out. And in fact, makemigrations should never be run with extensions loaded. Those migrations need to be created with care and should not affect the main codebase.

@frjo

frjo commented Sep 30, 2024

Copy link
Copy Markdown
Member

Will talk with OTF and other about this. My own first reaction is that a newsletter is not core to Hypha, feels more like a feature for each organisations public page.

@frankduncan

Copy link
Copy Markdown
Contributor Author

Yeah, for sure. The whole reason this is an extension, rather than a change to the base User model, is because it feels like something that doesn't belong in hypha. If you accept the PR in full (rather than just the parts that enable it to work), it would come defaulted to "off", and you could leave it in as an example to other's about how they might add their own such extensions. Or, if they want this checkbox to appear, it's here for them to turn on.

Our client wanted it here, though :) They don't have the bandwidth to manage a whole other site with its own user database, so this is a good placeholder.

@bickelj

bickelj commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

@frjo I think the extension approach is contained in the first five commits, and the next one is an example extension. It would be hard to review the extension in a vacuum thus the inclusion of the newsletter.

Does the extension approach look OK given what you see here?

@frjo

frjo commented Oct 9, 2024

Copy link
Copy Markdown
Member

We are discussing the extension system in the dev team. We might have alternatives we want to suggest. More info after meeting tomorrow.

@frankduncan

Copy link
Copy Markdown
Contributor Author

What came out of the discussions?

@frjo

frjo commented Nov 8, 2024

Copy link
Copy Markdown
Member

@frankduncan We are not moving forward with this approach. The benefits and potential of a plugin/extension system is however obvious seeing these PRs. The team are very interested in getting some system for this in place.

We have found https://github.com/simonw/djp an interesting solution.

@frjo frjo closed this Nov 8, 2024
@frankduncan

Copy link
Copy Markdown
Contributor Author

Even without the extensions, will you accept PRs that add signals and hooks into the main hypha codebase?

@frankduncan

Copy link
Copy Markdown
Contributor Author

FYI, my initial read on DJP is that it won't be able to do what we need it to do. Consider this extension. We need to be able to say in our tests that the hypha "edit user" page includes our new field. That means that the tests have to exist within the context of hypha, and the code needs to be able to modify hypha models. The extension has to be hypha-aware. That's the same for #4150.

DJP seems to be made for adding plugins into a system that is not aware of the system they are adding to. The tests exist independently of that system. Useful for saying "I want to add this system agnostic blogging component to my current application," but not "I want to optionally extend or modify my current application with an easy on/off switch." The latter is what we need.

Maybe my read on DJP is flawed, though. I just don't see how one would accomplish what's done here and in #4150 using it.

@frankduncan

Copy link
Copy Markdown
Contributor Author

Though maybe it would be the way forward if we were building a system and importing hypha as a library with a plethora of configuration and hooks a la wagtail, but as for developing plugins that we can import into the hypha application, that seems insoluble.

@frjo

frjo commented Nov 8, 2024

Copy link
Copy Markdown
Member

Moving plugin discussion here #4202

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 this pull request may close these issues.

3 participants