-
Notifications
You must be signed in to change notification settings - Fork 0
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
[7581] add user reports to moderation comment serializer #18
[7581] add user reports to moderation comment serializer #18
Conversation
hklarner
commented
Aug 9, 2023
•
edited
Loading
edited
- this PR depends on a PR on adhocracy4
- [7543] extends report serializer adhocracy4#1477
4d73289
to
52e51cc
Compare
Coverage reportTotal coverage
Report generated by 🧪jest coverage report action from 5ff8036 |
52e51cc
to
dc5b0b2
Compare
dc5b0b2
to
2f02595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, needs changelog and a test (at least add the user_reports
field to the test_fields()
I'd say)
2f02595
to
d8ec376
Compare
23669cd
to
e0c2819
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the names of the response data (received_data[n]
where n is 0 to 2) in the tests are a bit difficult to get in the assertions for which comments correspond.
I agree, it makes it harder to read and doesn't really reduce the line count. So I'm more in favor of easy readability here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, only thing to maybe fix is the readability of the test
e0c2819
to
5ff8036
Compare
rebase happens automatically when we merge if there are no conflicts with the main branch. |
I meant getting rid of the two commits "pull request review changes". |