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

Add validation for MAX_COMMIT_MESSAGE_LENGTH #11693

Closed
vojtechjelinek opened this issue Jan 14, 2021 · 11 comments · Fixed by #11871
Closed

Add validation for MAX_COMMIT_MESSAGE_LENGTH #11693

vojtechjelinek opened this issue Jan 14, 2021 · 11 comments · Fixed by #11871
Assignees

Comments

@vojtechjelinek
Copy link
Contributor

Add validation for the maximum commit length (MAX_COMMIT_MESSAGE_LENGTH) to the commit logs and snapshot metadata models.

@DubeySandeep
Copy link
Member

@vojtechjelinek is this issue already resolved?

@vojtechjelinek
Copy link
Contributor Author

@DubeySandeep No

@hardikkat24
Copy link
Contributor

@vojtechjelinek I would like to work on this issue.

@hardikkat24
Copy link
Contributor

@vojtechjelinek Can you tell me where should I start with? Thanks!

@vojtechjelinek
Copy link
Contributor Author

vojtechjelinek commented Feb 4, 2021

@hardikkat24 Hi, you will need to modify

class BaseSnapshotMetadataModelValidator(BaseSnapshotContentModelValidator):
and
class BaseCommitLogEntryModelValidator(BaseSnapshotMetadataModelValidator):
to include a check for the length of commit_message field in BaseSnapshotMetadataModel and BaseCommitLogEntryModel.

If this explanation is not enough please let me know.

@hardikkat24
Copy link
Contributor

@vojtechjelinek Thanks a lot! I have done as you said. But I have some questions:

  1. Should I add tests in prod_validators_test.py?
  2. How do i check that my validators are working correctly on my local machine?

@vojtechjelinek
Copy link
Contributor Author

@hardikkat24

  1. better to base_model_validators_test.py
  2. you start the local server, log in as admin, navigate to admin page and the jobs tab, run the jobs there

@hardikkat24
Copy link
Contributor

@vojtechjelinek Thanks a lot!
But what I did was I added validator for commit_message's length in BaseCommitLogEntryModelValidator and BaseSnapshotMetadataModelValidator.

Now I should test this code. So this can be done by adding tests in base_model_validators_test.py and running Jobs corresponding to ExplorationCommitLogEntryModelValidatorTests because I made changes in BaseCommitLogEntryModelValidator.
But base_model_validators_test.py doesn't have any present tests for other validators. So is there any other file where I must add the tests.
I'm a bit confused in this. Please clarify. Thanks!

@vojtechjelinek
Copy link
Contributor Author

@hardikkat24 Okay, put the tests in prod_validators_test.py, we can discuss any move of these tests to other places once you open a PR.

@hardikkat24
Copy link
Contributor

hardikkat24 commented Feb 6, 2021

@vojtechjelinek After I add the tests, then only the Jobs will show whether my approach was correct or not. Am I correct on this? Or can I test my approach without adding tests in the file.

@vojtechjelinek
Copy link
Contributor Author

As I said "you start the local server, log in as admin, navigate to admin page and the jobs tab, run the jobs there".

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

Successfully merging a pull request may close this issue.

3 participants