Radi85 / Comment

Django comment app. It can be associated with any given model.
https://rmustafa.pythonanywhere.com/
MIT License
97 stars 42 forks source link

Notification message via E-Mail #43

Closed frruit closed 3 years ago

frruit commented 4 years ago

Notification via E-Mail This is currently requested by my customer and i was wondering if this feature is maybe already in planning within the library itself. If not then I can maybe support. It would be great in that case to get a proposal from you how this should be done correctly

Describe the solution you'd like

What do you about this feature request?

Hope to hear soon from you. CYA

Radi85 commented 4 years ago

This feature is actually on my list after finishing #33 but with lesser details than you have mentioned, i.e. I didn't think of implementing thread's follower but it seems to be a nice feature alongside with email notifcation.

I would like to have this feature implemented including all the points you have mentioned and since you are aware of the whole workflow here, I will be looking forward to your PR :)

Radi85 commented 3 years ago

@abhiabhi94 I am working on this feature and I hope I will get enough time to finish it 😄 I have so far added a new model, I think it's called Follower to keep tracking of the subscribers of a comment. Just wanted to share the thoughts, any idea will be welcome!

abhiabhi94 commented 3 years ago

What all features are you considering in the first version of this?

Radi85 commented 3 years ago
abhiabhi94 commented 3 years ago

As this application becomes more and more complex on the front-end part, I see a reason why django-comments-xtd is using react :laughing:

That apart, why are you using another model for subscribers? Can't it be a many to many field with User model. `

Radi85 commented 3 years ago

As this application becomes more and more complex on the front-end part, I see a reason why django-comments-xtd is using react 😆

That is a nice solution. In fact we work on 2 different views for the same functionality (Django typical view and API view) I was thinking of using the API for our frontend as well and get rid of django typical views. However, this can be for later if we see this is really needed.

That apart, why are you using another model for subscribers? Can't it be a many to many field with User model. `

I used User model in the begnning but I took into consideration that we might extend this feature to allow anonymous to subscribe a model object, therefore I used a seperate model with email field along with content type fields.

abhiabhi94 commented 3 years ago

That is a nice solution.

No, no I am not sure I wouldn't would want to go that route. Sometime ago, when I wasn't really familiar with react, it was kind of intimidating to me to see that something like comment would use this. Now that I am a part of this, I see there are so many UI actions to handle :laughing: . Also, the overhead thing is an issue as well.

I used User model in the begnning but I took into consideration that we might extend this feature to allow anonymous to subscribe a model object, therefore I used a seperate model with email field along with content type fields.

Seems okay to me as of now. Also. I was thinking of updating the documentation regarding flags and some code cleaning/refactoring but will do it after this one is merged.

abhiabhi94 commented 3 years ago

Hey @Radi85, one more thing, do you like pytest? Now that I have used pytest-django, I like it. In general, I have found that we can write more test cases without having to actually write that much of code.

The fact that our existing test case will still work as before is an added advantage.

Radi85 commented 3 years ago

I did not use pytest previously and I don't mind to use it if you think that it can increase our code quality.

abhiabhi94 commented 3 years ago

it can help us in writing tests more easily. i am not too sure about the code quality though :)

one of the best things that i like about pytest-djsngo is that we have to explicitly mark tests that require database access, so not all tests have to go through setting up of database and then its teardown. i would recommend you too to give it a try if you have time. i will try to make a new branch and see the compatibility in our case.