feat: vocabularies' settings #1159

actionless · 2015-09-18T22:10:14Z

UPD:

sivakuna-aap · 2015-09-18T22:29:42Z

client/app/scripts/superdesk-vocabularies/vocabularies.js

+
+  VocabularyService
+      .$inject = [ 'api', 'urls', 'session', '$upload', '$q', 'metadata' ];
+  function VocabularyService(api, urls, session, $upload, $q, metadata) {


Unused injections $upload, metadata

actionless · 2015-09-18T22:32:24Z

@sivakuna-aap see https://dev.sourcefabric.org/browse/SD-694

sivakuna-aap · 2015-09-18T22:32:36Z

client/app/scripts/superdesk-vocabularies/vocabularies.js

+    $scope.vocabularies = null;
+    $scope.vocabulary = null;
+
+    $scope.createVocabulary = function() {


Since metadata.values is cached we either need to update the metadata.values or invalidate the metadata.values.

sivakuna-aap · 2015-09-18T22:41:24Z

@actionless after this ticket we had a discussion and finalized that vocabularies will not have UI. Not sure if this is been changed.

@akintolga @ioanpocol has this been discussed with Lasky or Vince about this ticket before adding this ticket to the current sprint?

actionless · 2015-09-20T22:19:49Z

@sivakuna-aap
mb for keywords we can just put directives from both PRs together into one settings.html? it shouldn't be a big issue to merge them together

sivakuna-aap · 2015-09-21T03:23:40Z

@actionless I had a discussion with @andrew Lasky and @akintolga. The keywords management will fall under the same UI you put in this PR. I will close my PR and rework after this is merged.

However, this PR needs to address below things:

When a vocabulary is updated then respective metadata.values should also be updated
Deleting an item in a vocabulary shouldn't delete the item from vocabulary.items. Instead it should change the value of is_active to false.

plamut · 2015-09-21T05:04:14Z

client/app/scripts/superdesk-vocabularies/tests/vocabularies.spec.js

+                expect(scope.vocabulary.items[0]).toEqual({foo: null, bar: null});
+                scope.removeItem({foo: null, bar: null});
+                expect(scope.vocabulary.items.length).toBe(0);
+            });


It would be better that the test for removing an item does not rely on the correct behavior of the scope.addItem() method - a bug in the latter could cause the former to fail as well, even though there would be nothing worng with the scope.removeItem() method.

A recommended approach is to directly create the desired fixture (i.e. setting the scope.vocabulary.items list), and run the method under tests (and nothing else) and observe the outcome.

plamut · 2015-09-21T05:17:00Z

The code looks good IMO, it just needs a few extra tests, docstrings (as @sivakuna-aap said) and perhaps some minor changes in the templates.

petrjasek · 2015-09-21T10:48:19Z

client/app/scripts/superdesk-vocabularies/tests/vocabularies.spec.js

+    it('can fetch vocabularies', inject(function(api, vocabularies, $q, $rootScope) {
+        spyOn(api, 'query').and.returnValue($q.when());
+        var scope = $rootScope.$new();
+        vocabularies.getVocabularies(scope);


the scope param is not used

actionless · 2015-09-21T18:09:26Z

@sivakuna-aap @plamut @petrjasek resolved comments

sivakuna-aap · 2015-09-21T21:33:23Z

client/app/scripts/superdesk-vocabularies/vocabularies.js

+            $scope._errorUniqueness = false;
+            api.save('vocabularies', $scope.vocabulary).then(onSuccess, onError);
+            // discard metadata cache:
+            metadata.initialize();


This won't force the metadata to reinitialize. https://github.com/superdesk/superdesk/blob/master/client/app/scripts/superdesk-authoring/metadata/metadata.js#L507

mdhaman · 2015-09-21T23:25:42Z

@actionless it is good to have vocabulary management.
imo, some of the vocabularies don't require management. For example, type, signal, subscriber_type.

There are few question that needs to be discussed before we introduce vocab management. What should happen to the system if we mark the type=text as inactive or delete it? What should happen if I add a new subscriber_type or remove existing subscriber_type?

imo, categories, default_categories, genre, rightsinfo, locators will require management but not sure about others.

ioanpocol · 2015-09-25T07:50:32Z

@actionless Do you need any help on this PR?

actionless · 2015-09-28T00:07:15Z

rebased on current master

plamut · 2015-09-28T06:21:29Z

Looking at the failed test I think these code changes have not caused them (at least not all of them), it is just our e2e tests failing randomly (again). One or two protractor tests that failed in my PR (basically just a minor CSS change) were the same that failed here, too, and every time a build was restarted, a different test (or a combination of them) failed.

Restarting build jobs enough times will IMO make the build pass eventually, though this is of course not a definite solution of any kind...

Update: It seems that (from a limited sample, though) that this build consistently fails on the test "monitoring view configure a stage and then delete the stage", and occasionally one or two other tests randomly fail on top of that.

ioanpocol · 2015-09-29T12:44:08Z

@actionless
Please rebase again, seams that some tests are old and don't contains the changes for desk management.

…ates; remove unused injects

…ith promise; fix some tests

…sed injects

actionless · 2015-09-29T13:45:37Z

rebased

ioanpocol · 2015-09-30T07:41:33Z

The issue is still there. I will fix it.

actionless · 2015-09-30T10:46:29Z

thanks, i have no idea how can it be related to these changes

ioanpocol · 2015-10-01T06:44:36Z

@actionless
Please disable "monitoring view configure a stage and then delete the stage" test until the issue SD-3371 will be fixed.

…ete the stage" test

actionless · 2015-10-01T12:01:42Z

now travis is passing

ioanpocol · 2015-10-01T12:04:11Z

cool, so we can merge it

amagdas · 2015-10-01T12:08:54Z

👍

feat: vocabularies' settings

sivakuna-aap mentioned this pull request Sep 18, 2015

(feat) [SD-3183] Using keywords as labels to group content arbitrarily #1160

Closed

sivakuna-aap reviewed Sep 18, 2015
View reviewed changes

actionless force-pushed the add-vocab-settings branch 4 times, most recently from 59ca152 to dc68a0c Compare September 19, 2015 09:07

plamut reviewed Sep 21, 2015
View reviewed changes

petrjasek reviewed Sep 21, 2015
View reviewed changes

actionless force-pushed the add-vocab-settings branch 2 times, most recently from 7e327eb to 81b694d Compare September 21, 2015 18:08

actionless force-pushed the add-vocab-settings branch from 81b694d to 061952f Compare September 21, 2015 18:10

sivakuna-aap reviewed Sep 21, 2015
View reviewed changes

actionless added 15 commits September 29, 2015 15:45

feat: vocabularies' settings

77f9a4f

fix(client: superdesk-vocabularies): table view; fix model retrieval

8fcdd88

refactor(client: superdesk-vocabularies): remove unused

8e80c9f

style(client: superdesk-vocabularies): make jshint happy

b2eb191

style(client: superdesk-vocabularies): jscs -x

93fa99b

fix(client: superdesk-vocabularies): remove logging

07d5a2d

fix(client: superdesk-vocabularies): auto-model detection

529b009

refactor(client: superdesk-vocabularies): isolate scopes; split templ…

4198030

…ates; remove unused injects

fix,test(client: superdesk-vocabularies): don't wrap getVocabularis w…

98f1d6e

…ith promise; fix some tests

fix(client: superdesk-vocabularies): template fixes

3170f12

feat,test(client: superdesk-vocabularies): invalidate metadata cache

86129af

docs(client: superdesk-vocabularies): add docstrings

ed7efe9

fix,test(client: superdesk-vocabularies): cancel editing vocabulary

d5dbf5b

fix(client: superdesk-vocabularies): one-time bind for id, remove unu…

9525486

…sed injects

fix(client: superdesk-vocabularies): metadata cache invalidation

dcbb4c5

actionless force-pushed the add-vocab-settings branch from 42081ce to dcbb4c5 Compare September 29, 2015 13:45

test(client): disable "monitoring view configure a stage and then del…

7fdf6d5

…ete the stage" test

ioanpocol added a commit that referenced this pull request Oct 1, 2015

Merge pull request #1159 from superdesk/add-vocab-settings

409bace

feat: vocabularies' settings

ioanpocol merged commit 409bace into master Oct 1, 2015

actionless deleted the add-vocab-settings branch October 1, 2015 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: vocabularies' settings #1159

feat: vocabularies' settings #1159

actionless commented Sep 18, 2015

sivakuna-aap Sep 18, 2015

actionless commented Sep 18, 2015

sivakuna-aap Sep 18, 2015

sivakuna-aap commented Sep 18, 2015

actionless commented Sep 20, 2015

sivakuna-aap commented Sep 21, 2015

plamut Sep 21, 2015

plamut commented Sep 21, 2015

petrjasek Sep 21, 2015

actionless commented Sep 21, 2015

sivakuna-aap Sep 21, 2015

mdhaman commented Sep 21, 2015

ioanpocol commented Sep 25, 2015

actionless commented Sep 28, 2015

plamut commented Sep 28, 2015

ioanpocol commented Sep 29, 2015

actionless commented Sep 29, 2015

ioanpocol commented Sep 30, 2015

actionless commented Sep 30, 2015

ioanpocol commented Oct 1, 2015

actionless commented Oct 1, 2015

ioanpocol commented Oct 1, 2015

amagdas commented Oct 1, 2015

feat: vocabularies' settings #1159

feat: vocabularies' settings #1159

Conversation

actionless commented Sep 18, 2015

sivakuna-aap Sep 18, 2015

Choose a reason for hiding this comment

actionless commented Sep 18, 2015

sivakuna-aap Sep 18, 2015

Choose a reason for hiding this comment

sivakuna-aap commented Sep 18, 2015

actionless commented Sep 20, 2015

sivakuna-aap commented Sep 21, 2015

plamut Sep 21, 2015

Choose a reason for hiding this comment

plamut commented Sep 21, 2015

petrjasek Sep 21, 2015

Choose a reason for hiding this comment

actionless commented Sep 21, 2015

sivakuna-aap Sep 21, 2015

Choose a reason for hiding this comment

mdhaman commented Sep 21, 2015

ioanpocol commented Sep 25, 2015

actionless commented Sep 28, 2015

plamut commented Sep 28, 2015

ioanpocol commented Sep 29, 2015

actionless commented Sep 29, 2015

ioanpocol commented Sep 30, 2015

actionless commented Sep 30, 2015

ioanpocol commented Oct 1, 2015

actionless commented Oct 1, 2015

ioanpocol commented Oct 1, 2015

amagdas commented Oct 1, 2015