Dashboard models #9

dweep123 · 2014-06-23T21:21:23Z

This PR depends on the django-cms PR

econchick · 2014-06-24T17:23:10Z

Again, address the first PR. Either close and reopen, or update.

Also, when submitting PRs, please only do "pieces". You've put in settings, templates, urls, and media. But this PR is referring to dashboard models.

econchick · 2014-06-24T21:40:55Z

systers_portal/dashboard/admin.py

+from dashboard.models import Resource
+from dashboard.models import Tag
+from dashboard.models import Resource_Type
+from dashboard.models import CommunityPages


Lines 3-8 are a little redundant. I would do:

from dashboard.models import ( SysterUser, Community, News, Resource, Tag, Resource_Type, CommunityPages)

econchick · 2014-06-24T21:49:57Z

In addition to the comments I've left, I'd suggest to not put both blank=True and null=True for any model that inherits from CharField or TextField, like URLField, EmailField, etc. This is because this allows the DB to have two values for "no data", both an empty string, and NULL. You can read more here (the first three answers are good).

BTW the default behavior is blank=False and null=False. So other than being very explicit, there's no reason to include them when they're false.

econchick · 2014-06-24T21:51:32Z

Ah nearly forgot - could you also put in tests? See @ana-balica 's pr #7 for example on how to setup tests for models.

dweep123 · 2014-06-24T21:58:48Z

Can a tag be of more than 30 characters? Should I change it for tags also ?

econchick · 2014-06-24T21:59:43Z

That makes sense, sure.

ana-balica · 2014-06-25T16:10:49Z

systers_portal/dashboard/models.py

+        return "{0} of {1} Community".format(self.title, self.community.name)
+
+
+class CommunityPages(models.Model):


Make it CommunityPage (singular form) to be consistent with other models naming convention.

ana-balica · 2014-06-25T16:42:12Z

While working on permissions I have figured out we need a model to represent also community requests/proposals.
@rosariorobinson can you send the ABI_Community_Proposal_Form.pdf referenced in the Appendix of the requirements document? Thank you :)

rosariorobinson · 2014-06-25T21:06:36Z

Document sent to entire team.

ana-balica · 2014-06-26T15:39:56Z

systers_portal/dashboard/models.py

+
+
+class SysterUser(models.Model):
+


It's not a PEP8 error, but why did you add a blank line before class definition?

econchick · 2014-06-27T22:44:08Z

@dweep123 - a few things in addition to my comments:

I don't see tests for the Tag and Resource_Type model
You have inconsistent indentation; mixing tabs and spaces too.
Do get in the habit of checking our Travis when submitting a PR, too; you'll see that this build failed with a lot of flake8 errors.

econchick · 2014-06-27T22:44:35Z

systers_portal/dashboard/models.py

+        return self.name
+
+
+class Resource_Type(models.Model):


This should be ResourceType - not snake cased.

dweep123 · 2014-06-30T10:56:49Z

@econchick , PR has been updated according to the comments above.
Still there are some warnings for line too long , but If I break those lines then that might be a odd way of breaking lines .
Please review these changes :)

ana-balica · 2014-07-01T13:30:03Z

requirements.txt

@@ -5,3 +5,4 @@ flake8==2.1.0
 psycopg2==2.5.3
 django-cms==3.0
 djangocms-text-ckeditor==2.1.6
+django-countries==2.1.2


It would be nice to keep the requirements in alphabetical order, same as pip freeze does.
Can you update that?

dweep123 · 2014-07-01T14:43:00Z

@ana-balica updated

ana-balica · 2014-07-01T14:48:16Z

requirements.txt

@@ -1,7 +1,8 @@
 Django==1.6.5
-Sphinx==1.2.2


Did you try running pip freeze? It lists first the packages with uppercase and then those with lowercase.
So usually it outputs something like this:

Django==1.6.5 Sphinx==1.2.2 django-cms==3.0 [and so on]

Now updated according to pip freeze :)

econchick · 2014-07-02T01:25:33Z

systers_portal/dashboard/tests.py

+        self.assertEqual(systeruser.blog_url, 'http://blog_url.com')
+        self.assertEqual(systeruser.homepage_url, 'http://homepage_url.com')
+        second_user = User.objects.create(username='user2', password='user2')
+        second_systeruser = SysterUser.objects.create(user=second_user)


second_systeruser is never actually used.

second_systeruser is corresponding syster user for second_user , It was created to check if the user entry is deleted then its corresponding syster user entry should also be deleted.

econchick · 2014-07-02T01:42:26Z

Hi @dweep123 - can you see https://travis-ci.org/systers/portal/ ? This PR fails the build, seen here. Please address all the flake errors.

Otherwise, after the flake errors, this looks good. Ping me when this is updated and I will merge. (a simple @ will be fine)

dweep123 · 2014-07-02T11:48:23Z

@econchick , updated
There is still one flake8 error , that "second_systeruser is never used" but I created that to check when a user is deleted its corresponding systeruser is also deleted ,
Should I remove that test ?

econchick · 2014-07-02T15:48:57Z

@dweep123 add # nopep8 comment at the end of the line to keep the test and have flake8 ignore it. Be sure to run flake8 before updating the PR to make sure it works. I'm on my mobile so I'm sorry I can't link you the docs, but if you google "flake8 ignore" it is the first entry.

econchick · 2014-07-02T15:50:05Z

@dweep123 add # nopep8 comment at the end of the line to keep the test and have flake8 ignore it. Be sure to run flake8 before updating the PR to make sure it works. I'm on my mobile so I'm sorry I can't link you the docs, but if you google "flake8 ignore" it is the first entry.

dweep123 · 2014-07-02T16:42:14Z

@econchick , updated. No flake8 errors now :)

econchick · 2014-07-02T16:50:45Z

Thanks @dweep123. @ana-balica can you check Travis's failure? I was able to look it up and only see a secret key error - do you know what's going on?

Once the Travis build passes I'll merge.

ana-balica · 2014-07-02T18:08:34Z

@econchick I know what is the problem. Since we are using secure env, on PR from forked repos, travis is not including this data, hence the SECRET_KEY is missing and the build fails. More here.

I will check if the workaround provided by travis works for us.

ana-balica · 2014-07-06T08:34:43Z

Are we going to use slugs or ids for representing models in views?

ana-balica · 2014-07-06T08:48:44Z

systers_portal/dashboard/models.py

+                                     related_name='member_of_community')
+    community_admin = models.ForeignKey(SysterUser, related_name='community')
+    parent_community = models.ForeignKey('self', blank=True, null=True)
+    website = models.URLField(max_length=30, blank=True)


max_length is rather short for URLs.
Same on lines 40, 41, 42.

Changed CommunityPages to CommunityPage Added tests for models deleted unwanted files Removed credentials Added tests for models Run flake8 Added tests for models Added tests for models Added tests for models Changed Line breaks Added tests for models Added Tests for models Added Tests for models requirements in alphabetical order Added Tests for models requirements in alphabetical order Added Tests for models Added tests for models Added tests for models Added tests for models Ignore never used flake8 errors Added slug field Added tests for models

dweep123 · 2014-07-07T22:00:46Z

@ana-balica @econchick updated.
Instead of using django cms page model can we use a model with placeholder fields which can be edited through django-cms front end editing , more here
If we use it Community Page model would be something like

class CommunityPage(models.Model):
    title = models.CharField(max_length=255)
    editable_content = PlaceholderField('editable_content')
    community = models.ForeignKey(Community)
    slug = models.SlugField(max_length=150, unique=True)

This is more simple and we can define our own urls for these pages if we use the above model and also cms page model also provides a lot of other options which I guess we don't want.
The problem with django-cms is that it allows front end editing only for staff users , If a user has permissions to edit a model but is not a staff user then won't be able to edit the model through front end.
If we have to allow that then either we have to set permissions for specific objects or change some code in django-cms where it checks whether the user is staff or not ,here and here

econchick · 2014-07-07T23:04:15Z

@dweep123 @ana-balica I think I understand placeholder fields, and if I understand correctly, switching to this would make sense.

It sounds like you can extend the auth backend (here) just fine, although UserProfiles are deprecated in Django IIRC.

@ana-balica what do you think? Also, you think you can help/work with Chitra re the auth permissions?

Something to think about: having a WYSIWYG or a Markdown editor for the user-facing editable content. Folks will want to format the content, I'm assuming, and not want to write in pure HTML (I don't even know how Django/Django-CMS would handle the text)

ana-balica · 2014-07-08T07:57:26Z

@econchick sure. But first it would be nice to review and merge what we have at the moment, since it becomes more and more complicated to work on new tasks which are dependent on pending PRs :)

I suppose we want to achieve row level permissions. This is not complicated, we can have a decorator obj_permission_required or a function to check if user has object permission. I didn't look much into Django-CMS, but my concern how are we going to use it on views already defined by Django-CMS?

There are Django-CMS permissions, but I need to see how flexible are those and if they meet our needs.

Recently I have discovered that tinymce, at last, has a decent theme, so we can use django-tinymce to provide a WYSIWYG editor.

econchick · 2014-07-08T16:24:35Z

So this doesn't break the build (besides the secret key issue). I'm going to merge it so you both can continue on.

Dashboard models

econchick reviewed Jun 24, 2014
View reviewed changes

ana-balica reviewed Jun 25, 2014
View reviewed changes

ana-balica mentioned this pull request Jun 25, 2014

Create user groups and assign permissions on post_syncdb signal #14

Merged

Added dashboard models

343861f

ana-balica reviewed Jun 26, 2014
View reviewed changes

econchick reviewed Jun 27, 2014
View reviewed changes

ana-balica reviewed Jul 1, 2014
View reviewed changes

ana-balica mentioned this pull request Jul 1, 2014

Add view decorators for checking membership, authorship and admin rights #16

Merged

econchick reviewed Jul 2, 2014
View reviewed changes

econchick mentioned this pull request Jul 2, 2014

close #11 - create separate requirements files for prod and dev #15

Merged

dweep123 mentioned this pull request Jul 3, 2014

Update testing settings #18

Merged

ana-balica reviewed Jul 6, 2014
View reviewed changes

econchick added a commit that referenced this pull request Jul 8, 2014

Merge pull request #9 from dweep123/dashboard_models

5fc3552

Dashboard models

econchick merged commit 5fc3552 into anitab-org:master Jul 8, 2014

econchick mentioned this pull request Jul 8, 2014

Social login documentation #22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard models #9

Dashboard models #9

dweep123 commented Jun 23, 2014

econchick commented Jun 24, 2014

econchick Jun 24, 2014

econchick commented Jun 24, 2014

econchick commented Jun 24, 2014

dweep123 commented Jun 24, 2014

econchick commented Jun 24, 2014

ana-balica Jun 25, 2014

ana-balica commented Jun 25, 2014

rosariorobinson commented Jun 25, 2014

ana-balica Jun 26, 2014

econchick commented Jun 27, 2014

econchick Jun 27, 2014

dweep123 commented Jun 30, 2014

ana-balica Jul 1, 2014

dweep123 commented Jul 1, 2014

ana-balica Jul 1, 2014

dweep123 Jul 1, 2014

econchick Jul 2, 2014

dweep123 Jul 2, 2014

econchick commented Jul 2, 2014

dweep123 commented Jul 2, 2014

econchick commented Jul 2, 2014

econchick commented Jul 2, 2014

dweep123 commented Jul 2, 2014

econchick commented Jul 2, 2014

ana-balica commented Jul 2, 2014

ana-balica commented Jul 6, 2014

ana-balica Jul 6, 2014

dweep123 commented Jul 7, 2014

econchick commented Jul 7, 2014

ana-balica commented Jul 8, 2014

econchick commented Jul 8, 2014

		return "{0} of {1} Community".format(self.title, self.community.name)


		class CommunityPages(models.Model):

Dashboard models #9

Dashboard models #9

Conversation

dweep123 commented Jun 23, 2014

econchick commented Jun 24, 2014

Choose a reason for hiding this comment

econchick commented Jun 24, 2014

econchick commented Jun 24, 2014

dweep123 commented Jun 24, 2014

econchick commented Jun 24, 2014

Choose a reason for hiding this comment

ana-balica commented Jun 25, 2014

rosariorobinson commented Jun 25, 2014

Choose a reason for hiding this comment

econchick commented Jun 27, 2014

Choose a reason for hiding this comment

dweep123 commented Jun 30, 2014

Choose a reason for hiding this comment

dweep123 commented Jul 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

econchick commented Jul 2, 2014

dweep123 commented Jul 2, 2014

econchick commented Jul 2, 2014

econchick commented Jul 2, 2014

dweep123 commented Jul 2, 2014

econchick commented Jul 2, 2014

ana-balica commented Jul 2, 2014

ana-balica commented Jul 6, 2014

Choose a reason for hiding this comment

dweep123 commented Jul 7, 2014

econchick commented Jul 7, 2014

ana-balica commented Jul 8, 2014

econchick commented Jul 8, 2014