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

Add organization to hearing #169

Merged
merged 1 commit into from
May 19, 2016
Merged

Add organization to hearing #169

merged 1 commit into from
May 19, 2016

Conversation

Rikuoja
Copy link
Contributor

@Rikuoja Rikuoja commented Apr 29, 2016

Required by #138

akx
akx reviewed May 2, 2016
View reviewed changes
democracy/models/base.py
class Organization(BaseModel):
name = models.CharField(verbose_name=_('name'), max_length=255)
admin_users = models.ManyToManyField(
User, blank=True, related_name='admin_organizations'
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use settings.AUTH_USER_MODEL directly here -- aliasing it to User makes the casual reader think it's not pluggable.

@akx
Copy link
Contributor

akx commented May 2, 2016

Missing tests.

What is the actual desired serialization format for organization? (Should it maybe be a SlugRelatedField?)

@akx
Copy link
Contributor

akx commented May 2, 2016

Also missing a migration.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch 2 times, most recently from 1e5543a to 1d23f3d Compare May 2, 2016 11:20
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.408% when pulling 1d23f3d on add-organization-to-hearing into a5a2584 on master.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from 1d23f3d to ce84be1 Compare May 2, 2016 12:14
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.409% when pulling ce84be1 on add-organization-to-hearing into a5a2584 on master.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from ce84be1 to fc93db4 Compare May 2, 2016 12:19
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.408% when pulling 5c377be on add-organization-to-hearing into a5a2584 on master.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from 5c377be to fd80015 Compare May 2, 2016 12:22
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.408% when pulling fd80015 on add-organization-to-hearing into a5a2584 on master.

akx
akx reviewed May 3, 2016
View reviewed changes
democracy/migrations/0012_auto_20160502_1219.py
@@ -0,0 +1,41 @@
# -*- coding: utf-8 -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the -n flag with makemigrations to specify the migration's name. (auto migration names are not very nice.)

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from fd80015 to 7e2f91f Compare May 3, 2016 09:52
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.408% when pulling 7e2f91f on add-organization-to-hearing into a5a2584 on master.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from 7e2f91f to 7b70c3f Compare May 13, 2016 10:20
@akx
Copy link
Contributor

akx commented May 13, 2016

@Rikuoja Rebase required.

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch 2 times, most recently from eb6901e to 13b6ae9 Compare May 16, 2016 14:06
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.246% when pulling 13b6ae9 on add-organization-to-hearing into 03416ce on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.246% when pulling 13b6ae9 on add-organization-to-hearing into 03416ce on master.

@akx akx assigned tuomas777 and unassigned akx May 17, 2016
@akx
Copy link
Contributor

akx commented May 17, 2016

Tossing the ball to @tuomas777 to review + merge.

tuomas777
tuomas777 reviewed May 18, 2016
View reviewed changes
democracy/models/base.py
name = models.CharField(verbose_name=_('name'), max_length=255, unique=True)
admin_users = models.ManyToManyField(
settings.AUTH_USER_MODEL, blank=True, related_name='admin_organizations'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this model shouldn't be in base.py as it isn't used as a basis for anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't realize that was the point :)

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from 13b6ae9 to 5b733b7 Compare May 18, 2016 10:04
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 97.07% when pulling 5b733b7 on add-organization-to-hearing into 272f58d on master.

tuomas777
tuomas777 reviewed May 19, 2016
View reviewed changes
democracy/migrations/0015_add_organization_model.py
('modified_at', models.DateTimeField(default=django.utils.timezone.now, editable=False, verbose_name='time of last modification')),
('published', models.BooleanField(db_index=True, default=True, verbose_name='public')),
('deleted', models.BooleanField(db_index=True, default=False, editable=False, verbose_name='deleted')),
('id', models.CharField(blank=True, help_text='You may leave this empty to automatically generate an ID', max_length=32, primary_key=True, serialize=False, verbose_name='identifier')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is a bit wrong because StringIdBaseModel has changed, I think correct would be ('id', models.CharField(editable=False, max_length=32, primary_key=True, serialize=False, verbose_name='identifier'))

@Rikuoja Rikuoja force-pushed the add-organization-to-hearing branch from 5b733b7 to ed11ad3 Compare May 19, 2016 08:11
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 97.07% when pulling ed11ad3 on add-organization-to-hearing into 272f58d on master.

@tuomas777
Copy link
Contributor

LGTM!

@tuomas777 tuomas777 merged commit a2d5a57 into master May 19, 2016
@Rikuoja Rikuoja deleted the add-organization-to-hearing branch July 20, 2017 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tuomas777 tuomas777

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Rikuoja @akx @coveralls @tuomas777