Skip to content
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

OC_VCategories - this time actually usable ;) #41

Merged
merged 81 commits into from Nov 13, 2012
Merged

Conversation

tanghus
Copy link
Contributor

@tanghus tanghus commented Oct 19, 2012

First let's get the lingo straight: Categories == Tags == Groups == whatever-you-use-to-categorize-with.

One of the reasons I started hacking on ownCloud, was that I constantly got annoyed by having duplicate/empty tags in KDE Kontact, and the early versions of Akonadi/Nepomuk certainly didn't help ;)
So I wanted to have a backend, that made that close to impossible, and should it happen you could do a full purge and rescan.

My first merge request back in January tried to do that, but I can see now that it wouldn't have worked properly, and I'm quite happy that we settled on the cut down version which saved a users categories in oc_preferences - even with all the bug reports it subsequently gave because of the value field being too small :-P

This time I think I got it right. Instead of being based on app name as the previous version, it now uses entity names (types) like 'event', 'task' or 'contact', just as the sharing backend.
I had to create two extra tables: 'vcategory' holding userid, type and category name. The second 'vcategory_to_object' (I know, but please suggest a better name) holds the relations between categoryid (auto-incremented in vcategory), objectid (from eg. contacts_cards) and the type name which together makes a unique index.

In order to avoid mass-duplication of code, I also introduced a new method insertIfNotExists() to OC_DB - with unit tests of course ;) I saw some from bug reports lately that other apps might benifit from that as well.

Contrary to most classes in ownCloud, OC_VCategories holds very few static methods and most often must be instantiated.

The basic use can be seen in the corresponding branch vcategories_db in the apps repository, where I have ported the Calendar and Contacts apps to use it - basically changing from using appname to type name and adding some calls for purging the db on user and/or object deletion - but for Contacts I have made a proof of concept implementation. I will submit a pull request for that branch shortly after this one.

While at it I used the opportunity to make a full rewrite of the client side code for the Contacts app, which currently is a huge pile of unmaintainable spaghetti code... It was my first take on writing client side web app code, so it has to go ;) Uses the categories backend to abstract from the addressbooks and use groups to organize contacts, which will give a much more intuitive UX especially for those poor sods using Apple software that can only deal with one addressbook ;)

I estimate the rewrite to be ~75% done and it will certainly be done and tested for OC 5. It's in the contacts_rework branch btw.

Well 'nuff said. Please have a look and find all the gotchas I've overlooked :-)

@tanghus
Copy link
Contributor Author

tanghus commented Nov 12, 2012

@owncloud-bot this is ok to test

@DeepDiver1975
Copy link
Member

Retest this

@DeepDiver1975
Copy link
Member

@tanghus this time jenkins really found something - looks like a query causes trouble on pg:
https://ci.tmit.eu/job/pull-request-analyser/84/console

Committing transaction/savepoint
query(1): COMMIT
MDB2 Error: already exists: _doQuery: [Error message: Could not execute statement]
[Last executed query: CREATE INDEX "uid_index" ON "oc__46ba_vcategory" ("uid")]

@tanghus
Copy link
Contributor Author

tanghus commented Nov 12, 2012

@DeepDiver1975 Now we've got something :-) I'm a bit unsure on how to setup autotest.sh for postgres. The file says: "createuser -P (enter username and password and enable superuser)". Should username be "oc_autotest" and what should password be?

@DeepDiver1975
Copy link
Member

@tanghus Let me have a look at it tonight - CU on IRC

@tanghus
Copy link
Contributor Author

tanghus commented Nov 13, 2012

Now tested heavily with mysql, postgres and sqlite? I've got an uptodate branch ready with Calendar and Contacts ported to use this version. Any comments before I merge @DeepDiver1975 @icewind1991 @bartv2 @georgehrke ?

@DeepDiver1975
Copy link
Member

As usual I'd prefer a clean merge history :hiding: 😈

@tanghus
Copy link
Contributor Author

tanghus commented Nov 13, 2012

Well whaddaya expect after 25 days ;)

@tanghus
Copy link
Contributor Author

tanghus commented Nov 13, 2012

But seriously for later(!) is it better to do a 'git fetch origin master' rather that 'git pull origin master' or won't it make a difference?

@DeepDiver1975
Copy link
Member

But seriously for later(!) is it better to do a 'git fetch origin master' rather that 'git pull origin master' or won't it make a difference?

This is a question to our git magicians! ;-)
@bartv2

@bartv2
Copy link
Contributor

bartv2 commented Nov 13, 2012

doing a 'git fetch origin master' does not change any branches, only a merge does that. A merge also happens when you do a pull without --rebase. If you want to test if it is still mergeable you could do a test merge and when there are no merge conflicts or simple merge conflicts do a reset of the current branch to the previous commit on that branch

@tanghus
Copy link
Contributor Author

tanghus commented Nov 13, 2012

Now my head hurts :-P

Can it be said in simple steps how the workflow is to avoid getting the 'merged branch master of ... into branch ...' that @DeepDiver1975 apparently doesn't like? ;)

tanghus added a commit that referenced this pull request Nov 13, 2012
OC_VCategories - this time actually usable ;)
@tanghus tanghus merged commit c353568 into master Nov 13, 2012
@lock lock bot locked as resolved and limited conversation to collaborators Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants