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

Newsletter owner groups #265

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ptitloup
Copy link

I created this branch to add owner group(s) to newsletter. If newsletter have group(s), only users of this groups (and superuser) can manage it and his messages, subscription and submision...
Newsletter groups are not required, it doesn't change anything if you don't want to use it.
Please let me know if it contains mistakes.
Regards

@coveralls
Copy link

coveralls commented Nov 28, 2018

Coverage Status

Coverage decreased (-1.7%) to 86.895% when pulling aef8b1d on ptitloup:ptitloup/add_group_to_newsletter into 30c3ec3 on dokterbob:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 87.028% when pulling 63648f5 on ptitloup:ptitloup/add_group_to_newsletter into 30c3ec3 on dokterbob:master.

@dokterbob
Copy link
Collaborator

Thanks for the contrib. This is quite the feature and I'm quite loaded with work right now.

Please bear with me that it might take up to several months for us to properly review this and get back to you. In the mean time, the PR will stay open so others are able to use it and (hereby) are also explicitly invited to give feedback!

Of particular interest to me is to assure that:

  1. It doesn't break any existing functionality.
  2. Doesn't decrease coverage.
  3. Is fully documented.

@dokterbob
Copy link
Collaborator

@claudep Any chance you could have a look at this one, so @ptitloup doesn't have to wait so long?

@claudep
Copy link
Collaborator

claudep commented Nov 28, 2018

Not sure if I'll find time, but for better chances, the patch should also be stripped from layout and convenience changes. @ptitloup, only change what is required for the new functionality. Then you can suggest formatting changes in a separate patch if you want.

class Migration(migrations.Migration):

dependencies = [
('auth', '0008_alter_user_username_max_length'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unrelated migration - please remove it from this PR


dependencies = [
('auth', '0008_alter_user_username_max_length'),
('newsletter', '0004_auto_20180407_1043'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please create a completely new migration based from the current master?

@@ -516,13 +620,14 @@ def get_urls(self):
# only used in this part of the admin. For now, leave them here.
if HAS_CBV_JSCAT:
my_urls.append(url(r'^jsi18n/$',
JavaScriptCatalog.as_view(packages=('newsletter',)),
name='newsletter_js18n'))
JavaScriptCatalog.as_view(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase on current master (this was fixed slightly different in another PR)

@dokterbob dokterbob changed the title Ptitloup/add group to newsletter Newsletter owner groups Oct 24, 2020
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.

5 participants