Add extras fields to discussion #1360
Conversation
l-vincent-l
force-pushed
the
add_discussions_extras
branch
from
b545845
to
a26a0ee
Compare
udata/tests/test_discussions.py
Outdated
| @@ -100,6 +100,44 @@ def test_new_discussion_missing_subject(self): | |||
| }) | |||
| self.assertStatus(response, 400) | |||
|
|
|||
| def test_new_discussion(self): | |||
There was a problem hiding this comment.
The function must be renamed (test_new_discussion_with_extras ?) because it's already used (and so one of the test is hiding the other)
udata/tests/test_discussions.py
Outdated
| @@ -100,6 +100,44 @@ def test_new_discussion_missing_subject(self): | |||
| }) | |||
| self.assertStatus(response, 400) | |||
|
|
|||
| def test_new_discussion(self): | |||
| self.app.config['USE_METRICS'] = True | |||
There was a problem hiding this comment.
As you're only testing the extras part, I think you can safely remove everything related to metrics
udata/tests/test_discussions.py
Outdated
| self.assert201(response) | ||
|
|
||
| dataset.reload() | ||
| self.assertEqual(dataset.metrics['discussions'], 1) |
There was a problem hiding this comment.
These 2 lines too (metrics related, can be removed)
l-vincent-l
force-pushed
the
add_discussions_extras
branch
from
a26a0ee
to
2204b8f
Compare
l-vincent-l
force-pushed
the
add_discussions_extras
branch
2 times, most recently
from
75ff993
to
8e15d01
Compare
|
Done. |
udata/tests/test_discussions.py
Outdated
| self.assertEqual(discussion.user, user) | ||
| self.assertEqual(len(discussion.discussion), 1) | ||
| self.assertIsNotNone(discussion.created) | ||
| self.assertIsNone(discussion.closed) |
There was a problem hiding this comment.
Why is it important to check that discussion.created and discussion.closed aren't in the discussion ?
There was a problem hiding this comment.
It's a copy/paste from the add_new_discussion test.
It could be deleted.
We need it for transport.data.gouv.fr It'll be only accessible through API for now
l-vincent-l
force-pushed
the
add_discussions_extras
branch
from
8e15d01
to
408bb20
Compare
|
What can we do for this PR to be merged? I see there's already a consensus. |
|
I’ll merge it. Forgot that @l-vincent-l probably does not have the permission to do so. |
We need it for transport.data.gouv.fr
It'll be only accessible through API for now
fix #1359