Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API validationerror response in CommentCreate view #316

Open
dest81 opened this issue Jun 16, 2021 · 6 comments
Open

API validationerror response in CommentCreate view #316

dest81 opened this issue Jun 16, 2021 · 6 comments
Milestone

Comments

@dest81
Copy link
Contributor

dest81 commented Jun 16, 2021

Here we use iterkeys and code returns only name of the field which hasn't passed validation.
Also I think we should return full dictionary with field name and error message like it's done in the django-rest-framework:

        if serializer.is_valid():
            response = super(CommentCreate, self).post(request, *args, **kwargs)
        else:
            return Response(serializer.errors, status=400)

But this could be breaking change for someone.

@danirus
Copy link
Owner

danirus commented Jun 17, 2021

I do agree. I think I did it so to ease the reading of non_field_errors in the ReactJS plugin. I expected only one of such errors, the "User not authenticated" (a bit stupid decision in retrospective). But there is no actual problem reading the dictionary instead of a string in the plugin.

The only concern I see, as you pointed out, is that it breaks backward compatibility.

It can be fixed in rel-3.0.0, but it's quite dirty yet. Also v3 is very different from v2, it heavily breaks backward compatibility. If you feel like taking a look and fix this particular piece of code, just py.test the CommentCreateTestCase class, otherwise you will run in many issues. The main difference between v2 and v3 is that there's no more render_xtdcomment_tree but a render_xtdcomment_list, which visually renders nested comments but using a queryset instead of a dictionary of comments. v3 does not use Twitter-Bootstrap, and the default JavaScript plugin will be vanilla code, non dependant on JS Frameworks. There is a new model CommentReaction to record likes/dislikes, etc. that replaces the use of CommentFlag, which I shouldn't have used in the first place. If all that doesn't hold you back, let me know and I will write here how you can set up the branch and check the demo/project (those in the example directory do not work anymore).

@danirus danirus added this to the 3.0.0 milestone Jun 18, 2021
@dest81
Copy link
Contributor Author

dest81 commented Jun 21, 2021

Hi Danirus,
Yes, send me please (or write here) instructions how to setup rel-3.0.0

@danirus
Copy link
Owner

danirus commented Jun 24, 2021

Clone the repository and checkout the rel-3.0.0 branch. Create a virtualenv.
Run make deps to install the dependencies of django-comments-xtd and dependencies for development.
Then cd into the demo directory, and create a .env_local file with these settings (you can pick the values from the settings.py module in the example/comp directory):

export DJANGO_SETTINGS_MODULE=project.settings_dev
export SECRET_KEY="your-django-project-secret-key"
export COMMENTS_XTD_SALT="salt-for-django-comments-xtd"

Then, yet in the demo directory, install the dependencies of the demo project: pip install -r requirements.txt.
Next, cd into the project directory and run sh install.sh. This creates the db by running the migrate command and loads the fixtures.
After that it should be ready for python manage.py runserver.

I haven't used the API since I started the branch, but most of the methods should work. The feedback method doesn't exist anymore. Instead I added an /api/react/ path to send reactions to comments. It must work but I haven't tested it yet.

Use py.test to run the tests, and make coverage to see tested code coverage report.
If you have suggestions, they are welcome.

@danirus
Copy link
Owner

danirus commented Jun 26, 2021

Just pushed changes in rel-3.0.0 to configure the tests.
There was an issue with the pytest.ini and the conftest.py module.

@dest81
Copy link
Contributor Author

dest81 commented Jul 16, 2021

Hi @danirus,
Sorry, was busy at work, currently on vacation, will have a look in it in two weeks. Hope it's not too late.

@danirus
Copy link
Owner

danirus commented Jul 17, 2021

It's never too late... :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants