Skip to content

Add messenger react app - #1

Merged
tehreem-sadat merged 1 commit into
developfrom
tehreem/add_messenger_backend_and_react_base_messenger_page
Sep 7, 2021
Merged

Add messenger react app#1
tehreem-sadat merged 1 commit into
developfrom
tehreem/add_messenger_backend_and_react_base_messenger_page

Conversation

@tehreem-sadat

@tehreem-sadat tehreem-sadat commented Sep 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@tehreem-sadat tehreem-sadat self-assigned this Sep 2, 2021
@tehreem-sadat
tehreem-sadat force-pushed the tehreem/add_messenger_backend_and_react_base_messenger_page branch from 29cb1d9 to 56d63de Compare September 2, 2021 12:12
@tehreem-sadat tehreem-sadat mentioned this pull request Sep 2, 2021
Merged
tasawernawaz
tasawernawaz reviewed Sep 2, 2021
View reviewed changes
Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mark this as a translation string.

tasawernawaz
tasawernawaz suggested changes Sep 2, 2021
View reviewed changes

@tasawernawaz tasawernawaz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks, @tehreem-sadat for working so hard on this in such a short time period and bringing this up.
Overall it is a great work, just a few suggestions, and comments.

Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why we need both id and username, usually it's not a good idea to expose the user's id.

Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

make this for translation

Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why we need this check

@tehreem-sadat tehreem-sadat Sep 3, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

as this is custom method, so we want to make sure that no one should be allowed to call this method without request object.

Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this serializer has common fields and methods as ConversationAccessSerializer, can we use inheritance here?

Comment thread openedx/features/wikimedia_features/messenger/api/v0/serializers.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe this should be done on the frontend side as this is related to the UI. If UI changes we don't want to get back here.

Comment thread openedx/features/wikimedia_features/messenger/api/v0/views.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we aren't consistent with this authentication class, this isn't being used in Bulk creation.

Comment thread openedx/features/wikimedia_features/messenger/models.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i belive Django automatically creates index on foreign keys,

@tehreem-sadat tehreem-sadat Sep 3, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Django create individual index but here we need composite index as mostly we will use query containing both sender and receiver.

Comment thread openedx/features/wikimedia_features/messenger/signals.py Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

just a suggestion you can clean this if by checking if not created at the top

Comment thread openedx/features/wikimedia_features/messenger/static/messenger/js/client/index.js Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: new line

Comment thread ...x/features/wikimedia_features/messenger/static/messenger/js/components/Conversation/index.js Outdated

@tasawernawaz tasawernawaz Sep 2, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

make sure once we get the actual design we will apply css through classes.

@tehreem-sadat
tehreem-sadat force-pushed the tehreem/add_messenger_backend_and_react_base_messenger_page branch from 56d63de to 1548515 Compare September 3, 2021 13:32
@tehreem-sadat

tehreem-sadat commented Sep 3, 2021

Copy link
Copy Markdown
Collaborator Author

@tasawernawaz I have done all suggested changes except 2 for which I have mentioned the reasons on your comments. Please review it again.

tasawernawaz
tasawernawaz approved these changes Sep 7, 2021
View reviewed changes

@tasawernawaz tasawernawaz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just one minor comment, otherwise LGTM

Comment thread openedx/features/wikimedia_features/messenger/api/v0/views.py Outdated

@tasawernawaz tasawernawaz Sep 7, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this docstring needs to be updated now

@tehreem-sadat
tehreem-sadat force-pushed the tehreem/add_messenger_backend_and_react_base_messenger_page branch from 1548515 to cdaead3 Compare September 7, 2021 07:16
@tehreem-sadat
tehreem-sadat merged commit e37fd21 into develop Sep 7, 2021
tehreem-sadat pushed a commit that referenced this pull request Sep 26, 2021
@maneeshatejani1
Merge pull request #1 from edlywmf/maneesha/new-django-app
47aa8f9 
create new django app for wikimedia
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@tasawernawaz tasawernawaz tasawernawaz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tehreem-sadat tehreem-sadat

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tehreem-sadat @tasawernawaz