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.

Sign up for GitHub

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

Jump to bottom

Apphook config multisite #5458

Merged
merged 1 commit into from
Jul 6, 2016
Merged

Apphook config multisite #5458

merged 1 commit into from
Jul 6, 2016

Conversation

yakky
Copy link
Member

@yakky yakky commented Jun 26, 2016 •
edited by czpython

Admittedly this was a choice made upon introduction of apphook config, but I can't see a good reason to not allow to reuse the same apphook config on different sites. application_namespace is already marked as unique only for each site (https://github.com/divio/django-cms/blob/release/3.3.x/cms/models/pagemodel.py#L124) thus I don't think apphook config should be more restrictive. I'd backport this at least to 3.2.x branch too

@yakky yakky added this to the 3.3.1 milestone Jun 26, 2016
@yakky yakky added status: ready for review forwardport Commits are being forward ported labels Jun 26, 2016
@yakky
Copy link
Member Author

yakky commented Jun 26, 2016

@divio/django-cms-core please review

@yakky yakky mentioned this pull request Jun 26, 2016
Closed
@coveralls
Copy link

coveralls commented Jun 26, 2016 •
edited

Coverage Status

Coverage remained the same at 88.913% when pulling 8c53b1e on yakky:feature/apphook_config_multisite into af20a56 on divio:release/3.3.x.

@yakky yakky mentioned this pull request Jun 26, 2016
Closed
@yakky
Copy link
Member Author

yakky commented Jun 28, 2016

@mkoistinen @czpython any chance we can discuss this?

@czpython
Copy link
Contributor

@yakky
Yes, I was reviewing over the weekend but didn't understand the change.

@yakky
Copy link
Member Author

yakky commented Jun 28, 2016

@czpython as of now, you can reuse the same application name over multiple sites (as unique together takes sites into account) https://github.com/divio/django-cms/blob/release/3.3.x/cms/models/pagemodel.py#L124
Apphook Configs, though, are restricted to be reused over multiple sites by a check in the form removed in this PR. There is no good reason to be more restrictive with apphooks configs that for "normal" application names

@evildmp
Copy link
Contributor

evildmp commented Jun 29, 2016

Hi @yakky can this be squashed into a single commit please and retitled, according to http://docs.django-cms.org/en/release-3.3.x/contributing/development-policies.html#commits?

Happy to help with wording, ping me on IRC etc.

@yakky
Copy link
Member Author

yakky commented Jun 29, 2016

@evildmp will do. @czpython @mkoistinen any more comment on this one?

@czpython
Copy link
Contributor

All good for me 👍

@yakky yakky force-pushed the feature/apphook_config_multisite branch from 8c53b1e to be0b258 Compare June 30, 2016 06:57
@coveralls
Copy link

coveralls commented Jun 30, 2016 •
edited

Coverage Status

Coverage remained the same at 88.768% when pulling be0b258 on yakky:feature/apphook_config_multisite into 04a6aec on divio:release/3.3.x.

@yakky
Copy link
Member Author

yakky commented Jul 1, 2016

@mkoistinen could you review this?

@mkoistinen
Copy link
Contributor

LGTM

@yakky
Copy link
Member Author

yakky commented Jul 5, 2016

Do we want a changelog entry for this?

@czpython
Copy link
Contributor

czpython commented Jul 5, 2016

I think it wouldn't hurt

@yakky yakky force-pushed the feature/apphook_config_multisite branch from a30e31b to 5ae5a1a Compare July 5, 2016 16:36
@yakky
Copy link
Member Author

yakky commented Jul 5, 2016

Added. Merging when tests complete

@coveralls
Copy link

coveralls commented Jul 5, 2016 •
edited

Coverage Status

Coverage remained the same at 88.768% when pulling be0b258 on yakky:feature/apphook_config_multisite into 04a6aec on divio:release/3.3.x.

@yakky yakky merged commit 630ead2 into django-cms:release/3.3.x Jul 6, 2016
This was referenced Jul 6, 2016
Merged
Merged
@razisayyed
Copy link

@yakky I am facing the same problem with 3.4.0

@czpython
Copy link
Contributor

@razisayyed Please create a new ticket with more information (traceback, requirements.txt, settings, etc.)

razisayyed added a commit to razisayyed/django-cms that referenced this pull request Sep 16, 2016
@razisayyed
Fix uniqueness ofnamespace per site
bcbe93d 
Filter by site_id to allow the namespace to be used in multiple sites.
Already fixed in previous versions.
django-cms#5458
@razisayyed razisayyed mentioned this pull request Sep 16, 2016
Merged
@razisayyed razisayyed mentioned this pull request Sep 16, 2016
Closed
razisayyed added a commit to razisayyed/django-cms that referenced this pull request Sep 16, 2016
@razisayyed
Make check unique namespace works in multisites
deb41de 
Filter based on site_id to allow namespace to be used in multisites.
Already fixed in previous versions of cms 
django-cms#5458
@razisayyed razisayyed mentioned this pull request Sep 16, 2016
Closed
@yakky yakky mentioned this pull request Sep 17, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
forwardport Commits are being forward ported status: ready for review
Projects
None yet
Milestone
3.3.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@yakky @coveralls @czpython @evildmp @mkoistinen @razisayyed