Update moderation API use #336
Conversation
Codecov Report
@@ Coverage Diff @@
## master #336 +/- ##
==========================================
- Coverage 76.56% 76.52% -0.04%
==========================================
Files 120 119 -1
Lines 5953 5948 -5
Branches 966 967 +1
==========================================
- Hits 4558 4552 -6
- Misses 1395 1396 +1
Continue to review full report at Codecov.
|
| annotationId: '<', | ||
| isReply: '<', | ||
| }, | ||
| bindings: moderationBanner.bindings, |
There was a problem hiding this comment.
This change ensures that the interface of the <moderation-banner> component (ie. its inputs) assumed by this test matches the actual interface.
ab01aee to
e053185
Compare
There was a problem hiding this comment.
This all looks pretty solid. I'll do some manual testing to make sure it works but want to see what you think of some requested changes first (and then if the changes are made, I'll test the final version):
My only suggestion is to maybe move a handful of methods (isHiddenByModerator(), flagCount(), isHidden()) into annotation-metadata and add unit tests to them there, both to avoid duplication and to have things more unit tested. Let me know what you think of this:
It seems like annotation-metadata is the place that we have to put functions that compute values based on an annotation's data, for example isReply(), isNew(), isPublic() are already in there, and isHiddenByModerator() etc seem to fit with that.
But also it seems like at least some components, for example moderation-banner, are tested using what I'd call an integration testing style where the tests work by looking for things in the rendered HTML and asserting against that.
For example the moderation-banner tests are things like 'displays the number of flags the annotation has received' that tests that the text content of an HTML element is 'Flagged for review x10'. If I break the moderation banner component's flagCount() method to always return 0 then four tests fail, most of these test failures don't seem relevant to the bug and none of them point me very accurately towards the line of code that's wrong, at least 3/4 of these test failures are quite misleading about what the bug is:
displays the number of flags the annotation has receiveddisplays in a more compact form if the annotation is a replyhides the annotation if "Hide" is clickedreports an error if hiding the annotation fails
Worse, if I break flagCount() to throw an exception instead then (after I fix an afterEach() that crashes and prevents most of the tests from running) 8 different tests fail due to a one-line bug.
I see this in the annotation component tests too - there's no direct unit tests for isHiddenByModerator(), if I break it to always return false then the test that fails is 'renders hidden annotations with a custom text class' which is a test that works by looking for a CSS class in the element's rendered HTML.
Again if isHiddenByModerator() is broken the test failure that I get isn't particularly "close" to isHiddenByModerator(), I don't think the test failure zeros in on the particular line of code that has the bug like direct unit tests of isHiddenByModerator() would.
And again if I break isHiddenByModerator() to do something completely different, for example throw an exception, then 75 tests fail (and none of the test failures points very accurately towards the line of code that's broken)
I think tests that work by looking at the HTML probably do make sense as a way of testing components, since quite a lot of what makes up the implementation of a component is its templates.
But I think the problem here is that we're putting too much logic into components, when we should be keeping them as simple as possible (for example with as little logic and as few different paths through the code as possible).
I think the solution to this might be:
-
Move logic like
isHiddenByModerator()into places likeannotation-metadatawhich is not a component but a bag of methods that are each unit tested directly. -
Continue to test components like the annotation and moderation-banner components in a more integrated style via their templates as we do currently, but for test isolation's sake consider using a mock annotation-metadata in these tests. (So that for example if
annotation-metadata.isHiddenByModerator()is broken then the unit tests for that method inannotation-metadata's tests fail, but dozens of component tests don't).
What do you think? (Sorry for the long explanation, I think it would actually be a small change to this PR.)
src/sidebar/components/annotation.js
Outdated
| if (!vm.annotation.moderation) { | ||
| return false; | ||
| } | ||
| return vm.annotation.moderation.is_hidden; |
There was a problem hiding this comment.
I think there's a test missing here. If I change this function to always return false then renders hidden annotations with a custom text class fails. But if I change it to always return true then nothing fails. I wonder if it needs a does not render the custom text class on not hidden annotations test?
| return [204, {}, {}]; | ||
| }); | ||
| $httpBackend.flush(); | ||
| }); |
There was a problem hiding this comment.
I wanted to play around with this test but it seems to crash when run on its own. It looks like maybe the before() in these tests is implicitly dependent on some other tests running first and creating the Angular "h" module. It's not a problem with this pull request (which just adds two new tests that are exactly like the existing ones), just something we might want to fix after.
diff --git a/src/sidebar/test/store-test.js b/src/sidebar/test/store-test.js
index b898b29..a55d912 100644
--- a/src/sidebar/test/store-test.js
+++ b/src/sidebar/test/store-test.js
@@ -150,7 +150,7 @@ describe('store', function () {
$httpBackend.flush();
});
- it('hides an annotation', function (done) {
+ it.only('hides an annotation', function (done) {
store.annotation.hide({id: 'an-id'}).then(function () {
done();
});$ make test
npm test
> hypothesis@1.9.0 test /home/seanh/Projects/client
> gulp test
[10:54:16] Using gulpfile ~/Projects/client/gulpfile.js
[10:54:16] Starting 'test'...
decorator { '0': [Function],
'1': 500,
'2': true,
'3': { level: 'debug', format: '%b %T: %m', terminal: true } }
START:
05 04 2017 10:54:25.718:INFO [framework.browserify]: bundle built
05 04 2017 10:54:25.747:INFO [karma]: Karma v1.1.0 server started at http://localhost:9876/
05 04 2017 10:54:25.747:INFO [launcher]: Launching browser PhantomJS with unlimited concurrency
05 04 2017 10:54:25.757:INFO [launcher]: Starting browser PhantomJS
05 04 2017 10:54:25.991:INFO [PhantomJS 2.1.1 (Linux 0.0.0)]: Connected on socket /#iumxM0kD00OwoEd9AAAA with id 97156479
Finished in 0.118 secs / 0 secs
SUMMARY:
✔ 0 tests completed
✖ 1 test failed
FAILED TESTS:
store
✖ "before all" hook
PhantomJS 2.1.1 (Linux 0.0.0)
[$injector:nomod] Module 'h' is not available! You either misspelled the module name or forgot to load it. If registering a module ensure that you specify the dependencies as the second argument.
http://errors.angularjs.org/1.5.6/$injector/nomod?p0=h
/node_modules/angular/angular.js:2103:0 <- /tmp/d6a061fd0af4d6d989b337980871d659.browserify:6818:71
ensure@/node_modules/angular/angular.js:2025:0 <- /tmp/d6a061fd0af4d6d989b337980871d659.browserify:6740:45
module@/node_modules/angular/angular.js:2099:0 <- /tmp/d6a061fd0af4d6d989b337980871d659.browserify:6814:20
/tmp/sidebar/test/store-test.js:14:0 <- /tmp/d6a061fd0af4d6d989b337980871d659.browserify:109988:19
[10:54:26] 'test' errored after 10 s
[10:54:26] Error: 1
at formatError (/home/seanh/Projects/client/node_modules/gulp/bin/gulp.js:169:10)
at Gulp.<anonymous> (/home/seanh/Projects/client/node_modules/gulp/bin/gulp.js:195:15)
at emitOne (events.js:96:13)
at Gulp.emit (events.js:191:7)
at Gulp.Orchestrator._emitTaskDone (/home/seanh/Projects/client/node_modules/orchestrator/index.js:264:8)
at /home/seanh/Projects/client/node_modules/orchestrator/index.js:275:23
at finish (/home/seanh/Projects/client/node_modules/orchestrator/lib/runTask.js:21:8)
at cb (/home/seanh/Projects/client/node_modules/orchestrator/lib/runTask.js:29:3)
at removeAllListeners (/home/seanh/Projects/client/node_modules/karma/lib/server.js:379:7)
at Server.<anonymous> (/home/seanh/Projects/client/node_modules/karma/lib/server.js:390:9)
at Object.onceWrapper (events.js:293:19)
at emitNone (events.js:91:20)
at Server.emit (events.js:188:7)
at emitCloseNT (net.js:1562:8)
at _combinedTickCallback (internal/process/next_tick.js:77:11)
at process._tickDomainCallback (internal/process/next_tick.js:128:9)
npm ERR! Test failed. See above for more details.
Makefile:16: recipe for target 'test' failed
make: *** [test] Error 1|
This is looking good to me. I think I still need to manually test it, that'll have to wait until tomorrow morning. |
The `hide` API has now been implemented on the server. The `unhide` API has been agreed but not yet implemented.
… annotation After an annotation has been hidden or unhidden by a moderator, the moderation state needs to be updated locally to reflect the change.
In the `<moderation-banner>` and `<annotation>` components, read the moderation state from the annotation object instead of fetching it from the app state.
Moderation state was originally going to be retrieved via a separate API call from /api/search but is now returned as a `moderation` field in the annotation response. This commit removes the separate `moderation` state from the Redux store.
The previous code was incorrect because depending on the failure, the error might not be an `Error`. This needs to be fixed in the API client but for the moment, sidestep the problem by just displaying a fixed string.
The `hidden` state has been moved from the `annotation.moderation` key to a top-level property of the annotation.
Move the flag count to a helper which is easier to test and re-use.
90f0fdb to
b71a6ff
Compare
|
Rebased. I've tested this locally and it all seems to work |
Update logic for reading and updating moderation state to reflect new APIs
Moderation state was originally going to be retrieved via a separate API
call from /api/search but is now returned as a
moderationfield in theannotation response.
This PR simplifies and updates the client to reflect this.
moderationstate (insrc/sidebar/reducers/moderation) from the Redux store since this state is now part of the annotation itself.moderationproperty (seesrc/sidebar/components/moderation-banner)src/sidebar/reducers/annotations) to update the hidden state of an annotation after a call to hide or unhide the annotation succeeds. This is needed because the hide/unhide API calls do not return the updated annotation content.src/sidebar/store)Error-like.