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

Joyous EventBase.status field clashes with Django-Model-Utils StatusModel.status field #35

Open
linuxsoftware opened this issue Aug 11, 2020 · 5 comments · May be fixed by #34
Open

Joyous EventBase.status field clashes with Django-Model-Utils StatusModel.status field #35

linuxsoftware opened this issue Aug 11, 2020 · 5 comments · May be fixed by #34
Milestone

Comments

@linuxsoftware
Copy link
Owner

@sam-mi reports:

Event.status fieldname clashes with Django Model Utils Model.status field making subclasses of ls.joyous EventBase incompatible with any project that already implements Model Utils StatusField in their models.

I assume https://github.com/jazzband/django-model-utils is the correct homepage for Django Model Utils?

Pull request #34

This would break backwards compatibility for Joyous so would have to go in a 2.0 release.

@linuxsoftware
Copy link
Owner Author

I have never used django-model-utils, but would it not be easier just to use a StatusField with a different name instead of using the StatusModel mixin?
e.g.

class DeliveryPage(joyous.models.EventBase):
    delivery_status = StatusField(_('status'))
    delivery_status_changed = MonitorField(_('status changed'), monitor='delivery_status')

etc?

@linuxsoftware linuxsoftware added this to the 2.0 milestone Aug 11, 2020
@linuxsoftware linuxsoftware linked a pull request Aug 11, 2020 that will close this issue
@linuxsoftware linuxsoftware changed the title Event.status field clashes with StatusModel.status field EventBase.status field clashes with StatusModel.status field Aug 11, 2020
@linuxsoftware linuxsoftware changed the title EventBase.status field clashes with StatusModel.status field Joyous EventBase.status field clashes with Django-Model-Utils StatusModel.status field Aug 11, 2020
@linuxsoftware
Copy link
Owner Author

Closing this issue as I have had no response

@sam-mi
Copy link

sam-mi commented Aug 31, 2020

Sorry, I got pulled away.

No, it would not be easier because that breaks interoperability with other libraries that already rely on ModelUtils via model mixins etc. Django Model Utils is an established library that has set a precedent in the Django community that status on a model is generally a CharField that stores values the the DB rather than a property method that calculates a value.

Besides it's much easier to modify the name of a calculated property than to change the name of a model field that is inherited from an installed library, especially once migrations have been run.

I think this simple change is valuable as it improves Joyous compatibility with an existing and well established library and as such improves interoperability with other apps as well, which can only help the uptake of LS.Joyous - which is an excellent library btw!

@sam-mi
Copy link

sam-mi commented Aug 31, 2020

I have another branch that I could submit a PR from if you think it would be useful?

It's a much larger change - refactoring models as abstract models that can be extended in the final project - no field changes, no migrations, updated tests, custom templates etc, actually quite possibly all non-breaking changes. I have done this to allow me to integrate registration and ticket sales on event pages within my wagtail implementation - which was not possible using the current version of LS.Joyous, at least not while extending my other page types as well.

@linuxsoftware linuxsoftware reopened this Aug 31, 2020
@linuxsoftware
Copy link
Owner Author

Where I would like to get to is where there was just one Event type and one Exception type. There is a discussion document on this in the wiki. Code changes towards that goal would be welcome. This will be a major change though, so will need to wait until 2.0.

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