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

Don't allow to create subprojects with same alias #5404

Merged
merged 7 commits into from Apr 4, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Mar 6, 2019

Added a clean_alias method in the ProjectRelationshipBaseForm which will check if the alias already exists. This will prevent Projects from Creating New subprojects with same alias.
closes #5402

@saadmk11 saadmk11 changed the title Added clean method for subproject alias. Don't allow to create subprojects with same alias Mar 6, 2019
Copy link
Member

@stsewd stsewd left a comment

Looks good, but we need to add some tests for this before merging.

readthedocs/projects/forms.py Outdated Show resolved Hide resolved
@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 6, 2019

@stsewd Okay. I'll add some tests and update the PR.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 7, 2019

@stsewd I have Added some Tests and Updated the PR.

Copy link
Member

@stsewd stsewd left a comment

Thanks for the tests, but we already have some of them here https://github.com/rtfd/readthedocs.org/blob/78c34c904b347110b2cd545b4b5a80ed526590f7/readthedocs/rtd_tests/tests/test_subprojects.py#L13-L13 can you move the new tests there?

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 8, 2019

@stsewd Thanks for pointing it out. I couldn’t find the existing test case as the name is different. I'll move the tests here and update the PR.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 8, 2019

@stsewd Moved the tests to test_subprojects.py

stsewd
stsewd approved these changes Mar 11, 2019
Copy link
Member

@stsewd stsewd left a comment

I think some tests are already there, but the test for the alias is correct 👍

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 11, 2019

@stsewd Should I remove the tests that are already there or this can be merged?

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 11, 2019

You can try to identify the repeated tests, but it's fine for me.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 11, 2019

@stsewd okay cool.

@saadmk11
Copy link
Member Author

@saadmk11 saadmk11 commented Mar 16, 2019

@stsewd Removed Repeated tests. :)

@stsewd stsewd requested a review from Mar 21, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Apr 4, 2019

Tested this locally, it's working

@stsewd stsewd merged commit ee5ffd0 into readthedocs:master Apr 4, 2019
1 check passed
@saadmk11 saadmk11 deleted the subproject-alias branch Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants