-
Notifications
You must be signed in to change notification settings - Fork 209
Conversation
@yatna I have applied the filter for sorting. |
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.
Everything looks good, I suggest you add prefixes to all api routes... When this is merged I would create a base test which you can use a template.
selenium==3.12.0 |
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.
Ps, why does this appear as an addition?
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 might have backspaced and retyped- doesn't make a difference though
systers_portal/meetup/urls.py
Outdated
@@ -135,4 +135,5 @@ | |||
DeleteSupportRequestCommentView.as_view(), | |||
name="delete_support_request_comment"), | |||
url(r'^(?P<slug>[\w-]+)/(?P<meetup_slug>[\w-]+)/$', MeetupView.as_view(), name="view_meetup"), | |||
url(r'^request_meetup_data/$', api_for_vms, name='api_for_vms'), |
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.
Why don't you add and API prefix? like api/ to make it clearer that one is dealing with API routes?
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.
How about adding a vX
prefix where X = version of our API, I've seen this followed quite much.
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.
Yes makes sense something like api/vX
(api/v1/request_meetup_data
, api/v2/request_meetup_data
....)
@fenn-cs @Monal5031 I've added the prefix. Please have a look now. |
@yatna . I made some changes suggested by @fenn-cs and @Monal5031 . Please can u approve again as the previous approval turned stale. |
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.
LGTM 👍
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.
Everything looks ok :)
bbcb4c3
to
789732b
Compare
839235f
to
cc3e7a9
Compare
|
||
|
||
@api_view(['POST']) | ||
def api_for_vms(request): |
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.
Instead of function based views, please switch this to class based views as it has higher capabilities.
Merge pull request anitab-org#403 from abhi20161997/favicon_replace Replacing the favicon transfered and rewrote base, login and admin tests from automated testing Signed-off-by: Fenn-25 <fenn25.fn@gmail.com> api to communicate with VMS
Description
Sending meetup data like event name, date and venue to VMS
Fixes #411
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
Describe the tests you ran to verify your changes. Provide instructions or GIFs so we can reproduce. List any relevant details for your test.
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only