🐛(backend) enforce emoji validation for reactions#1965
🐛(backend) enforce emoji validation for reactions#1965lunika merged 1 commit intosuitenumerique:mainfrom
Conversation
maboukerfa
force-pushed
the
fix/enforce_emoji
branch
from
d709426 to
f2b1da9
Compare
lunika
left a comment
lunika
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution.
Just one comment, and it will be good.
|
|
||
| def validate_emoji(self, value): | ||
| """Ensure the reaction is a single emoji.""" | ||
| if not emoji.is_emoji(value): |
There was a problem hiding this comment.
Can you please add a warning log here? To ensure that valid emoji are not rejected?
There was a problem hiding this comment.
I added a warning log to for the rejected emojis and their corresponding Unicode
maboukerfa
force-pushed
the
fix/enforce_emoji
branch
from
f2b1da9 to
ca38eaa
Compare
|
Hey @lunika, any updates on this? |
Thank you for your patience. About emoji, we would like to localize them. Will your validation still work once it's made? Thanks. |
|
Alright, thanks for the update! Yes, the validation should still work as long as the localization is handled on the front end. On the backend, I’m using the Unicode encoding for emojis, so it remains consistent regardless of how they’re implemented in the front. I'll check it again once the localization implemented |
Validate emojis in ReactionSerializer (previously accepted any string), preventing multiple emojis or text uploads in a single reaction Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
lunika
force-pushed
the
fix/enforce_emoji
branch
from
ca38eaa to
162dbe5
Compare
WalkthroughThis pull request adds emoji validation for reactions across the codebase. The changes include: adding the Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/tests/documents/test_api_documents_comments.py (1)
647-842:⚠️ Potential issue | 🟠 MajorThese tests now exercise “react to existing emoji” more than “create reaction”.
Using
ReactionFactory(comment=comment)to source the request emoji pre-creates a reaction row, so many “create reaction” tests no longer validate first-time creation behavior. Use a known valid emoji literal (or a non-persisted helper value) for request payloads.✅ Suggested test pattern
- reaction = factories.ReactionFactory(comment=comment) + emoji_value = "🐛" @@ - {"emoji": reaction.emoji}, + {"emoji": emoji_value}, @@ - assert models.Reaction.objects.filter( - comment=comment, emoji=reaction.emoji, users__in=[user] - ).exists() + assert models.Reaction.objects.filter( + comment=comment, emoji=emoji_value, users__in=[user] + ).exists()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/documents/test_api_documents_comments.py` around lines 647 - 842, Tests currently use ReactionFactory(comment=comment) to pick an emoji for the POST payload which pre-creates a Reaction row and makes the tests exercise "react to existing emoji" instead of creating a new one; change each test that sets reaction = factories.ReactionFactory(comment=comment) solely to obtain an emoji to instead use a literal emoji string (e.g. "👍") or a non-persisted helper value and remove the ReactionFactory usage where not asserting existing-reaction behavior so that the endpoint creates a new Reaction (adjust tests like test_create_reaction_authenticated_user_public_document, test_create_reaction_authenticated_user_accessible_public_document, test_create_reaction_authenticated_user_connected_document, test_create_reaction_authenticated_user_restricted_accessible_document, and parametrized cases accordingly); keep ReactionFactory only in tests that explicitly assert behavior for an already-existing reaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 205: Move the changelog line "- 🐛(backend) enforce emoji validation for
reactions `#1965`" out of the v4.7.0 release section and insert it under the "##
[Unreleased]" section; locate the string "- 🐛(backend) enforce emoji validation
for reactions `#1965`" and the headings "## [Unreleased]" and "## [v4.7.0]" in
CHANGELOG.md, remove the entry from the v4.7.0 block and add it (preserving
formatting) to the Unreleased section.
In `@src/backend/core/factories.py`:
- Around line 234-237: The ReactionFactory's emoji field currently uses
factory.Faker("emoji") causing nondeterministic test data; change the default to
a deterministic, valid emoji (e.g., "👍") on the ReactionFactory class so tests
are stable while still allowing callers to override the value when they need
randomness (update the emoji attribute on ReactionFactory from
factory.Faker("emoji") to a fixed emoji default).
---
Outside diff comments:
In `@src/backend/core/tests/documents/test_api_documents_comments.py`:
- Around line 647-842: Tests currently use ReactionFactory(comment=comment) to
pick an emoji for the POST payload which pre-creates a Reaction row and makes
the tests exercise "react to existing emoji" instead of creating a new one;
change each test that sets reaction = factories.ReactionFactory(comment=comment)
solely to obtain an emoji to instead use a literal emoji string (e.g. "👍") or a
non-persisted helper value and remove the ReactionFactory usage where not
asserting existing-reaction behavior so that the endpoint creates a new Reaction
(adjust tests like test_create_reaction_authenticated_user_public_document,
test_create_reaction_authenticated_user_accessible_public_document,
test_create_reaction_authenticated_user_connected_document,
test_create_reaction_authenticated_user_restricted_accessible_document, and
parametrized cases accordingly); keep ReactionFactory only in tests that
explicitly assert behavior for an already-existing reaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16120f49-e9d1-495f-9a40-30fb63f8b00c
📒 Files selected for processing (5)
CHANGELOG.mdsrc/backend/core/api/serializers.pysrc/backend/core/factories.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/pyproject.toml
|
|
||
| ### Fixed | ||
|
|
||
| - 🐛(backend) enforce emoji validation for reactions #1965 |
There was a problem hiding this comment.
Move this changelog entry to Unreleased instead of v4.7.0.
This item is currently recorded under a past release (v4.7.0 on March 9, 2026), but the PR was opened on March 10, 2026. It should be tracked under ## [Unreleased] until release cut.
📝 Proposed changelog fix
### Fixed
- 🚸(frontend) redirect on current url tab after 401 `#2197`
- 🐛(frontend) abort check media status unmount `#2194`
- ✨(backend) order pinned documents by last updated at `#2028`
- 🐛(frontend) fix app shallow reload `#2231`
- 🐛(frontend) fix interlinking modal clipping `#2213`
- 🛂(frontend) fix cannot manage member on small screen `#2226`
- 🐛(backend) load jwks url when OIDC_RS_PRIVATE_KEY_STR is set
+- 🐛(backend) enforce emoji validation for reactions `#1965`
...
### Fixed
-- 🐛(backend) enforce emoji validation for reactions `#1965`
- 🐛(frontend) analytic feature flags problem `#1953`
- 🐛(frontend) fix home collapsing panel `#1954`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 205, Move the changelog line "- 🐛(backend) enforce
emoji validation for reactions `#1965`" out of the v4.7.0 release section and
insert it under the "## [Unreleased]" section; locate the string "- 🐛(backend)
enforce emoji validation for reactions `#1965`" and the headings "## [Unreleased]"
and "## [v4.7.0]" in CHANGELOG.md, remove the entry from the v4.7.0 block and
add it (preserving formatting) to the Unreleased section.
| skip_postgeneration_save = True | ||
|
|
||
| comment = factory.SubFactory(CommentFactory) | ||
| emoji = "test" | ||
| emoji = factory.Faker("emoji") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer a deterministic default emoji in ReactionFactory.
Using factory.Faker("emoji") as the default can introduce avoidable test nondeterminism. A fixed valid emoji default (with explicit override when randomness is needed) will keep reaction tests stable.
♻️ Suggested refactor
class ReactionFactory(factory.django.DjangoModelFactory):
@@
- emoji = factory.Faker("emoji")
+ emoji = "🐛"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| skip_postgeneration_save = True | |
| comment = factory.SubFactory(CommentFactory) | |
| emoji = "test" | |
| emoji = factory.Faker("emoji") | |
| skip_postgeneration_save = True | |
| comment = factory.SubFactory(CommentFactory) | |
| emoji = "🐛" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/factories.py` around lines 234 - 237, The ReactionFactory's
emoji field currently uses factory.Faker("emoji") causing nondeterministic test
data; change the default to a deterministic, valid emoji (e.g., "👍") on the
ReactionFactory class so tests are stable while still allowing callers to
override the value when they need randomness (update the emoji attribute on
ReactionFactory from factory.Faker("emoji") to a fixed emoji default).
2 participants
Purpose
This pull request enforces emoji validation in the
ReactionSerializerto prevent invalid emoji submissions.Proposal
ReactionSerializer(previously accepted any string)