Enable/disable addressbooks by skjnldsv · Pull Request #412 · nextcloud/contacts

skjnldsv · 2017-10-27T20:28:42Z

skjnldsv · 2017-10-27T20:32:13Z

 <div class="contacts-list" ng-class="{loading: ctrl.loading, 'mobile-show': ctrl.show}">
        <div class="app-content-list-item"
-			 ng-repeat="contact in ctrl.filteredContacts = (ctrl.contactList | contactGroupFilter:ctrl.routeParams.gid | localeOrderBy:ctrl.sortBy | filter:query | limitTo:ctrl.limitTo ) as filtered track by contact.uid()"
+			 ng-repeat="contact in ctrl.filteredContacts = (ctrl.contactList | filter:{'data':{'addressBook':{'enabled': true}}} | contactGroupFilter:ctrl.routeParams.gid | localeOrderBy:ctrl.sortBy | filter:query | limitTo:ctrl.limitTo) as filtered track by contact.uid()"


For some reason it doesnt work! :/
@Henni ?

As far as I can see, you only store the state in localstorage and don't update it inside the addressbook itself.
This update is only applied on page reload (https://github.com/nextcloud/contacts/pull/412/files#diff-adfefcaa5c468a1b21ca840b249cadcfR14).

Why don't you store it on the server in the first place?

I don't think we can.

Should be possible, calendar does it. But they use a property in the 'owncloud' namespace.

<d:propertyupdate xmlns:d="DAV:"><d:set><d:prop><o:calendar-enabled xmlns:o="http://owncloud.org/ns">0</o:calendar-enabled></d:prop></d:set></d:propertyupdate>

@irgendwie Is such a thing possible with our current dav lib?

Oooh yeah, I remember this! I don't see where it's stored in the db though! :/

owncloud/contacts#409 (comment)

~~IIRC, the settings weren't saved.~~

Okay, it's in oc_properties and it is beeing saved!

Though it's not loaded on propfind

Okay, I needed a patch for the dav lib.
Easy fix now! :)

All good :)

Henni · 2017-10-28T10:08:57Z

 .factory('AddressBook', function()
 {
 	return function AddressBook(data) {
+		var storageKey = btoa(data.data.href.split('/').splice(-3, 2).join('_'));


This variable isn't used later on.

Henni · 2017-10-28T10:26:31Z

 			readOnly: data.data.props.readOnly === '1',
+			// use only owner and url, convert ''/remote.php/dav/addressbooks/users/admin/Test1/'' to 'admin.Test'
+			key: data.data.href.split('/').splice(-3, 2).join('_'),
+			enabled: window.localStorage.getItem('contacts_ab_'+this.key) !== false,


this doesn't work, as localstorage only stores Strings.
=> this statement always evaluates to true.

Hum, it's strange since it actually remembers the state, so It seems to work :)

Henni · 2017-10-28T12:17:19Z

+			}
+		}
+	};
+});


this file shouldn't be in here ;)

Henni · 2017-10-28T12:17:34Z

+			<h2>{{ctrl.t.emptySearch}}</h2>
+		</div>
+	</div>
+</div>


this as well

Henni · 2017-10-28T13:48:06Z


+	ctrl.toggleState = function() {
+		ctrl.enabled = AddressBookService.toggleState(ctrl.addressBook);
+		ContactService.fillCache();


Why do you call fillCache?

It was to force a full refresh of the cache. This pr is still wip, I'm not convinced yet! :)

codecov · 2017-10-29T05:13:57Z

Codecov Report

Merging #412 into master will decrease coverage by 0.84%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
- Coverage   16.62%   15.78%   -0.85%     
==========================================
  Files          61       61              
  Lines        1311     1381      +70     
==========================================
  Hits          218      218              
- Misses       1093     1163      +70

Impacted Files	Coverage Δ
js/models/addressBook_model.js	`100% <ø> (ø)`	⬆️
...mponents/contactImport/contactImport_controller.js	`5% <0%> (-1.25%)`	⬇️
...s/components/addressBook/addressBook_controller.js	`1.05% <0%> (-0.06%)`	⬇️
...s/components/contactList/contactList_controller.js	`0.74% <0%> (-0.06%)`	⬇️
js/services/addressBook_service.js	`0.8% <0%> (-0.25%)`	⬇️
js/services/contact_service.js	`0.49% <0%> (-0.07%)`	⬇️
...onents/contactDetails/contactDetails_controller.js	`1.96% <0%> (ø)`	⬆️
js/models/contact_model.js	`38.38% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72b9423...3b03a70. Read the comment docs.

Done

skjnldsv · 2017-11-13T08:47:58Z

Bump! :)
@nextcloud/contacts

Henni · 2017-11-13T12:58:03Z

Works mostly fine.

Also moving contacts seems to have a few issues at the moment:

For some reason the currently selected contact is changed when enabling/disabling addressbooks:
Moving a contact to an addressbook and disabling & enabling this addressbook hides the contact but doesn't reshow it.
Also moving contacts to a different addressbook breaks opening them in the details view.

Both of these issues are fixed after reloading the page.

@skjnldsv Can you reproduce this?

skjnldsv · 2017-11-18T07:38:21Z

Let me fix this, and then publish a new version! :)

skjnldsv · 2017-11-20T04:52:53Z

@Henni Should be fixed :)

Edit: still an issue left:

Enabling doesn't update groups

…ot synced Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Add notify on contact import from addressbook Fix Groups update on enabling/disabling Changed default ab to first that is writtable instead of first of the list Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

fixup! Fix contact check add/remove to ab

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2017-11-21T15:08:15Z

Pfiouuu! Rebased and ready to merge!
@nextcloud/contacts @jancborchardt @MorrisJobke

skjnldsv · 2017-12-06T06:16:35Z

Bump?

irgendwie

Seems to work fine, changes make sense.

private-nc-user · 2018-02-12T07:59:17Z

I just noticed, that i can't disable a shared adressbook. The adressbook is shared to me, disabling the originating adressbook works. Since I'm a newbie can you just give me an advice how to open an issue the right way. Or is just writing this comment enough?

timreeves · 2018-02-12T18:37:56Z

I have also just noticed the same problem as @private-nc-user . It goes like this:

Request = https://owncloud.{hidden}/remote.php/dav/addressbooks/users/{hidden}/gabriele_aktiv_shared_by_Gabriele/ (Method = PROPPATCH)
Response = 403 Forbidden

skjnldsv · 2018-02-13T04:35:55Z

Yes, you can't disable a shared addressbook as the disabled permission is for the owner of this addressbook :/

timreeves · 2018-02-13T07:25:36Z

Thanks for the explanation John @skjnldsv. It's just not what we wanted to hear )-: Look at it from the user pov, not the core hacker pov: The address book shared with me by someone else is more likely the one I want to be able to blend out the most, as it does not contain the contacts which I myself have gathered.
I understand that the backend features you build on do not allow to do what you want; but in that case, either there is a feature lacking, or a layer of abstraction somewhere. Or the app needs to take more control itself and add the neccessary abstraction, or whatever.
Don't take this too bad - I really really do appreciate all the work contributors do, really - but right now I'm feeling like a guy who bought a car and then found the motor built in onto the passenger seat - sorry mate, it was the only place we could fit it in :)

mburnicki · 2018-02-13T08:47:55Z

I also think that it would benice that if you "disable" a specific address book then the contacts from that address books are simply not shown. I'm currently maintaining about 30 address books that are shared with different groups of users, so it would be nice to "enable" just a single address book to see only the contacts in that address book to maintain those contacts.

This is how e.g. the "cardbook" addon for Thunderbird
https://addons.mozilla.org/en-US/thunderbird/addon/cardbook/
works, and this is also possible in the "kontact" app that ships with KDE.

Also, a contact search should only be done in address books that are "enabled".

If you disable the address books on the server then other clients will also be unable to sync the contacts from the disabled address books. There may be cases where this is also desired, but usually you can even configure in Android's contact app which address books to synchronize, and addresses from which address books to be displayed in the contacts list.

timreeves · 2018-02-13T10:23:25Z

Very lucid comment from @mburnicki - he says it better than I did. Seconded! What we need is a clear distinction between disabling an address book completely (I never imagined I might not be able to sync it from my Thunderbird if it is disabled in NC GUI display - igitt); and simply removing its data from the current GUI display settings.

I would personally not even mind if it were not displayed (or even prefer) but still searchable when I type in part of a name or email, e.g. in the Mail app.

What we need to remember is that for some users this hiding of address books in the NC GUI only ever came up as an issue because of the way the app is designed - to always fetch ALL the contacts data to the browser and then implement user display choices in JS. This gives you nifty features for privaze-size address books, but breaks down at around 1000 contacts, becomes too slow to use. So it is not workable for professional-size / team / company address books.

Thus I still advocate a choice of "front-end" OR "backend with real infinite scrolling" solutions to display and manage the contacts, see this issue. Quoting from that: What I really miss is the option to select WHICH of those three books are to be displayed in the GUI: I would only really need one in everyday use, the other two are my archive and my girlfriends book, both of which I only seldom need.

So imho it was never about disabling an address book completely (although that is also a nice feature), but rather about choosing which to display in the GUI. But a seldom-used address book, which I do not want shown in the GUI, should still be available in the background for syncing or more intensive use by others. Put simply, I just want to choose for myself not to see my girlfriends contacts - I could never tell her to disable her address book.

The very minimum we need here is a clear notice in the GUI that disabling an address book will also prevent it from being synced (if I have understood that right).

Anyway, thanks to all involved, as ever!

skjnldsv · 2018-02-13T13:13:26Z

I'll see what I can do, it's maybe doable, I don't know. Can one of you guys open an issue about this? :)

Henni · 2018-02-13T18:48:56Z

@timreeves @mburnicki thanks for the constructive comments. as @skjnldsv said, please open an issue about this and we see what we can do :)

skjnldsv · 2018-02-13T18:51:00Z

@timreeves @mburnicki thanks for the constructive comments

Yes, it's always a pleasure to have people like you being so nice and friendly about issues issues! :)

timreeves · 2018-02-13T21:17:07Z

To make sure we don't open two: I'm starting on the new issue now.

mburnicki · 2018-02-14T08:11:23Z

@timreeves Thanks. Yesterday I was too busy to do it.

skjnldsv added the 2. developing Work in progress label Oct 27, 2017

skjnldsv self-assigned this Oct 27, 2017

skjnldsv force-pushed the enable-disable-ab branch 2 times, most recently from 4fd643c to e9d4e82 Compare October 27, 2017 20:31

skjnldsv commented Oct 27, 2017

View reviewed changes

Henni requested changes Oct 28, 2017

View reviewed changes

Henni reviewed Oct 28, 2017

View reviewed changes

Henni previously requested changes Oct 28, 2017

View reviewed changes

Henni reviewed Oct 28, 2017

View reviewed changes

skjnldsv force-pushed the enable-disable-ab branch from 21bc0b8 to aa592eb Compare October 29, 2017 04:31

skjnldsv requested review from Henni, irgendwie and jancborchardt November 1, 2017 16:54

skjnldsv added 3. to review Waiting for reviews enhancement New feature or request and removed 2. developing Work in progress labels Nov 1, 2017

skjnldsv force-pushed the enable-disable-ab branch from cd84f74 to 7e80a41 Compare November 1, 2017 17:01

skjnldsv requested review from MorrisJobke, jonatoni and xh3n1 November 3, 2017 17:55

skjnldsv added the Hackweek label Nov 3, 2017

skjnldsv force-pushed the enable-disable-ab branch 2 times, most recently from cb04df0 to bb3aef0 Compare November 20, 2017 04:52

skjnldsv added the 2. developing Work in progress label Nov 20, 2017

skjnldsv added 8 commits November 21, 2017 15:55

Do not select nearest contact on enable and fetch contacts if ab is n…

d76ab80

…ot synced Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed addressbook model for contacts and move selector 1

0961929

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Move selector 2

2bc5768

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Prevent import if no addressbook is available

ba49483

Add notify on contact import from addressbook Fix Groups update on enabling/disabling Changed default ab to first that is writtable instead of first of the list Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fix contact check add/remove to ab

bad9889

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fixed contact index search and fixed contact cache update on enable

0410fba

fixup! Fix contact check add/remove to ab

Removed console log

7f82451

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Only select nearest contact if not in the list

1a41481

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the enable-disable-ab branch 2 times, most recently from e15dfde to cbb2864 Compare November 21, 2017 15:05

Rebase fail fixup

3b03a70

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the enable-disable-ab branch from cbb2864 to 3b03a70 Compare November 21, 2017 15:05

jonatoni approved these changes Dec 6, 2017

View reviewed changes

irgendwie approved these changes Dec 7, 2017

View reviewed changes

skjnldsv merged commit 2225d86 into master Dec 7, 2017

skjnldsv deleted the enable-disable-ab branch December 7, 2017 16:44

timreeves mentioned this pull request Feb 13, 2018

Add new option to hide an address book's contacts without disabling the address book #479

Closed

Conversation

skjnldsv commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Henni Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skjnldsv commented Nov 13, 2017

Uh oh!

Henni commented Nov 13, 2017 • edited by skjnldsv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Nov 18, 2017

Uh oh!

skjnldsv commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Nov 21, 2017

Uh oh!

skjnldsv commented Dec 6, 2017

Uh oh!

irgendwie left a comment

Choose a reason for hiding this comment

Uh oh!

private-nc-user commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timreeves commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Feb 13, 2018

Uh oh!

timreeves commented Feb 13, 2018

Uh oh!

mburnicki commented Feb 13, 2018

Uh oh!

timreeves commented Feb 13, 2018

Uh oh!

skjnldsv commented Feb 13, 2018

Uh oh!

Henni commented Feb 13, 2018

skjnldsv commented Oct 27, 2017 •

edited

Loading

skjnldsv Oct 29, 2017 •

edited

Loading

skjnldsv Oct 29, 2017 •

edited

Loading

Henni Oct 28, 2017 •

edited

Loading

codecov bot commented Oct 29, 2017 •

edited

Loading

Henni commented Nov 13, 2017 •

edited by skjnldsv

Loading

skjnldsv commented Nov 20, 2017 •

edited

Loading

private-nc-user commented Feb 12, 2018 •

edited

Loading

timreeves commented Feb 12, 2018 •

edited

Loading