-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some lines which need to be changed. Mixins and GenericRelations are used well. Thanks.
updated = models.DateTimeField(auto_now=True) | ||
|
||
|
||
class Tag(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should create a mixin for this too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a new PR later on.
|
||
def create(self, validated_data): | ||
owner = self.context.get("request").user | ||
content_type_object = ContentType.objects.get(model=validated_data['content_type']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.get(item)
is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the following lines as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it doesn't matter in our situation because it that field is missing, validators catch that and code don't even react this point.
# If 'artists' key is given in data, clear current artists | ||
# and add new ones. Else, discard it. | ||
artists_data = None | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try-except
is expensive. Instead, this structure is better:
artists_data = validated_data.get('artists')
if artists_data is not None:
pass
# do your stuff here
# pop if you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -0,0 +1,17 @@ | |||
from django.conf.urls import url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django 2.0+ provides better way for defining URLs. django.urls.path
can be used instead of django.conf.urls,url
. In this way, no regex is needed.
It is optional though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change it later, we're too busy with other things...
url(r'^attendance/(?P<pk>[0-9]+)/$', views.AttendanceView.as_view()), | ||
url(r'^comments/(?P<pk>[0-9]+)/$', views.CommentView.as_view()), | ||
url(r'^events_list/$', views.EventListView.as_view()), | ||
url(r'^events/(?P<pk>[0-9]+)/$', views.EventView.as_view()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During my tests, I noticed that /api/events/
does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it..
|
||
urlpatterns = [ | ||
path('admin/', admin.site.urls), | ||
path('api-auth/', include('rest_framework.urls')) | ||
path('api-auth/', include('rest_framework.urls')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this.
This PR contains implementation of
Event
,AttendanceStatus
,Comment
,FollowStatus
,Media
,Tag
andVoteStatus
models and their corresponding API endpoints.Location
model was also in the scope of this PR, however, since it's dependent on #77, it'll be implemented later.There might be few things missing in this PR. Frontend or Android teams might want to request data in a filtered manner or in a different structure than the current one. These points can be discussed and implemented accordingly. Another thing is about validations. I implemented certain checks and validations according to our requirements and class diagram, but of course along the way, other validation requirements may emerge. We can implement those as needed.