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

Limit snippet characters #379

Merged
merged 33 commits into from
Nov 17, 2023
Merged

Limit snippet characters #379

merged 33 commits into from
Nov 17, 2023

Conversation

freedompraise
Copy link
Collaborator

Description

Added functionality to ensure the snippet field has a limit.
The limit used was 255 other suggestions would be appreciated.
For more detail, it closes #365

Changes Made

  • Added new feature
  • Fixed bug
  • Refactored code
  • Other (please describe):

Test Plan

No test plan yet, till the logic is confirmed

Checklist

  • I have read the contributing guidelines
  • I have added tests to cover my changes and they all pass in addition to the existing tests.
  • I have added documentation for my changes (if appropriate)

@freedompraise freedompraise requested a review from jsolly November 9, 2023 04:27
@jsolly
Copy link
Owner

jsolly commented Nov 9, 2023

Looks good to me. I added a small CSS update so the error message looks red when there are too many characters.

Could you add a test for adding a snippet that is too long and it is blocked?

Copy link
Owner

@jsolly jsolly left a comment

Choose a reason for hiding this comment

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

Add one test for this

@freedompraise
Copy link
Collaborator Author

Add one test for this

Alright

@freedompraise freedompraise requested a review from jsolly November 10, 2023 03:27
app/tests/test_models.py Outdated Show resolved Hide resolved
app/blog/utils.py Outdated Show resolved Hide resolved
app/blog/utils.py Outdated Show resolved Hide resolved
@freedompraise freedompraise requested a review from jsolly November 13, 2023 13:32
@jsolly
Copy link
Owner

jsolly commented Nov 13, 2023

I made the following changes.
1 - Move the snippet validation logic to the Post forms.py file. This way we can add the validator without a new migration.
2 - Created a new test_validators.py testfile
3 - Refactored the snippet validator so max_length can be a dependency injection. We want to minimize using globals so the test functions can manipulate this value if they need to.

@jsolly
Copy link
Owner

jsolly commented Nov 13, 2023

@freedompraise unfortunately, I think the Snippet validator needs more work. I was able to get it to block me when I was well under 400 characters. This is because the text I copy/pasted in had formatting. See the following GIF
2023-11-13_13-57-22 (1)

Here is a link to the video if that GIF doesn't render.

Also added a failing test for you that contains the HTML shown in the video.

Copy link
Owner

@jsolly jsolly left a comment

Choose a reason for hiding this comment

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

Please resolve the failing text and check any other edge cases if you can find them.

@jsolly
Copy link
Owner

jsolly commented Nov 13, 2023

If you applied that migration you had, you'll have to roll that back because I removed it.

@freedompraise
Copy link
Collaborator Author

Please resolve the failing text and check any other edge cases if you can find them.

I'll look into it

@freedompraise freedompraise requested a review from jsolly November 16, 2023 02:49
@jsolly
Copy link
Owner

jsolly commented Nov 17, 2023

Nice work @freedompraise. Merging

Copy link
Owner

@jsolly jsolly left a comment

Choose a reason for hiding this comment

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

Approve!

@jsolly jsolly merged commit dcf2bc4 into master Nov 17, 2023
2 checks passed
@jsolly jsolly deleted the limit-snippet-characters branch November 17, 2023 17:53
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 this pull request may close these issues.

Limit Characters for Post Snippet
2 participants