Preliminary frontend work #13

PVince81 · 2016-12-14T11:57:20Z

Preliminary work for the frontend.

This extends the Makefile significantly and adds the following:

composer deps (currently only phpunit)
nodejs deps (karma + bower + handlebars)
bower deps
karma test runner
handlebars compilation

PVince81 · 2016-12-19T10:14:56Z

REQUIRES: enhance oc-backbone-webdav adapter to support MKCOL for creating collections (default is POST or PUT): Backbone Webdav adapter MKCOL support core#26842

PVince81 · 2016-12-19T10:20:54Z

REQUIRES: update handlebars version in core to avoid conflict Update handlebars core#26841

PVince81 · 2017-02-03T12:53:12Z

Rebased.

Will require adjustments to be able to work with the updated endpoint structure.

PVince81 · 2017-02-08T16:58:34Z

This "uri" approach is a nightmare.

Backbone (and potentially other frameworks) assume that if the id is known, then the object already exists. I have used the "uri" as id so it always uses the "update" internal method instead of "create" and tries to PROPPATCH instead of MKCOL.

I think we'll still need to use the actual internal id field as the backbone id, and the uri is only used for the path. I'll try that approach then.

PVince81 · 2017-02-08T17:29:14Z

REQUIRES: WebdavNode backbone models and collections core#27119 to be able to create with MKCOL
REQUIRES: Restrict proppatch to the proper nodes core#27118 to be able to rename

PVince81 · 2017-02-08T17:48:03Z

BUG: the Webdav endpoint must return the internal ID as header after MKCOL, for Backbone to be able to set an internal id

PVince81 · 2017-02-23T14:00:17Z

So I finally have a solution, but it required me to write custom model and collections specifically for Webdav. This would make it reusable in the future. See owncloud/core#27119.

The frontend now works, you can now:

create/rename/delete groups
leave a group
rename a group
add/delete members
switch role for a member

PVince81 · 2017-03-07T12:58:20Z

Latest state:

PVince81 · 2017-03-07T18:33:50Z

This is now ready for review.

Note that this core PR is still required to test this: owncloud/core#27119 (make sure to run make in core again to get the updated lib)

Everything is squashed into two commits:
The first commit is about improving the base stuff with a better Makefile (try make help). I hope to make this reusable for other apps.
The second commit adds all the JS code + JS unit tests to make the UI work as seen in #13 (comment)

Please review @felixheidecke @jvillafanez @DeepDiver1975 @VicDeo @IljaN

PVince81 · 2017-03-07T18:35:04Z

... and if you're wondering why I used window.prompt and window.confirm:
this here is the crude version of the UI. It is functional, but doesn't provide the best UX yet.
UX will be improved in subsequent PRs.

At least we could say this is a POC that could be used for demoing the functionality.

PVince81 · 2017-03-08T07:49:14Z

ah of course, unit tests cannot pass until owncloud/core#27119 from core is merged

jvillafanez · 2017-03-08T07:36:47Z

css/app.css

+/* FIXME: integrate with core styles */
+#customgroups .sidebar .close {
+	position: absolute;
+    top: 0;


indentation

jvillafanez · 2017-03-08T07:42:50Z

js/App.js

+				customGroupsTitle: t('customgroups', 'Custom Groups')
+			}));
+
+			this.$groupsContainer = this.$('.groups-container').removeClass('icon-loading');


I'm not sure if this.$('.groups-container') is a good idea. This is confusing for me.

If it's for marking the variable as a jquery object, I'd rather use a name to reflect that, for example "this.jqueryGroupsContainer"

this.$('.groups-container') is a Backbone View shortcut for this.$el.find('.groups-container')

As for this.$groupsContainer the $ sign is a convention that already tells that this attribute is a jQuery object, just like this.$el.

Hope that makes sense.

jvillafanez · 2017-03-08T07:48:39Z

js/App.js

+
+		_onSelectGroup: function(group) {
+			this.$membersContainer.empty();
+			var state = {


Is this variable used? The scope should be limited to this function and it isn't used here.

forgot to kill it...

jvillafanez · 2017-03-08T08:29:39Z

js/GroupsView.js

+				this,
+				'_onSubmitCreationForm',
+				'_onRenameGroup',
+				'_onDeleteGroup'


include the _onSelect function?

actually I'll remove them all because backbone auto-binds when specified in the "events" block

jvillafanez · 2017-03-08T08:36:52Z

js/GroupsView.js

+				// TODO: lock row during save
+				model.save({
+					displayName: newName
+				});


I guess the function should return true if it's renamed properly

no, this is a jquery event handler and the return value indicates whether bubbling is needed (or something like this).

Actually we don't need to return because we already call ev.preventDefault(); at the top.

I'll still add add return false;

jvillafanez · 2017-03-08T09:51:24Z

js/templates/list.handlebars

@@ -0,0 +1,27 @@
+<form name="customGroupsCreationForm">
+	<div>
+		<input


I'd rather "oneline" this if it fits. There is only one variable attribute.

ok. now it has 134 columns 😉

jvillafanez · 2017-03-08T09:53:32Z

js/templates/membersList.handlebars

+</div>
+<table class="grid">
+	<thead>
+		<th></th>


What is the purpose of these empty headers?

This header is for the avatar column

maybe an HTML comment here to make sure these header are in use? Just looking exclusively at the file it seems they can be deleted.

nah... will only do if I need to retouch this PR or next time I come around this place

an experienced HTML dev would know that one does not simply remove an empty "th" or "td" because that would mess up the layout 😉

jvillafanez · 2017-03-08T09:59:43Z

lib/Controller/PageController.php

+	 */
+	public function index() {
+		// TODO: cache or add to info.xml ?
+		$modules = json_decode(file_get_contents(__DIR__ . '/../../js/modules.json'));


I guess this "modules.json" file is used in more places, otherwise it would be better to hardcode the modules here.

The second usage is in the unit tests, that's why I wanted a separate file... I'm still longing for a proper portable way to load JS files for OC apps.

let's discuss in #34

jvillafanez · 2017-03-08T10:01:05Z

lib/Dav/CustomGroupsPlugin.php

@@ -91,7 +91,8 @@ public function initialize(\Sabre\DAV\Server $server) {
 		$this->server->xml->namespaceMap[self::NS_OWNCLOUD] = 'oc';
 		$ns = '{' . self::NS_OWNCLOUD . '}';
 		$this->server->resourceTypeMapping[MembershipNode::class] = $ns . 'customgroups-membership';
-		$this->server->resourceTypeMapping[GroupsCollection::class] = $ns . 'customgroups-group';
+		$this->server->resourceTypeMapping[GroupsCollection::class] = $ns . 'customgroups-groups';


"customgroups-groups" or "customgroups-group"? Is this intended?

This is intended, but not specifically for the frontend work... just a "pass-by" fix.

"customgroups-groups" plural is for GroupsCollection as the collection is a list of groups, so "groups".
The next item GroupMemberShipCollection is actually a single group, so it has the type "customgroups-group" singular.

Still, thanks for spotting

jvillafanez · 2017-03-08T10:02:34Z

lib/Dav/MembershipNode.php

@@ -36,7 +36,6 @@ class MembershipNode implements \Sabre\DAV\INode, \Sabre\DAV\IProperties {

 	const PROPERTY_ROLE = '{http://owncloud.org/ns}role';
 	const PROPERTY_USER_ID = '{http://owncloud.org/ns}user-id';
-	const PROPERTY_GROUP_URI = '{http://owncloud.org/ns}group-uri';


Any reason to remove this? Mainly to confirm this isn't accidental

The property isn't used at all in this node.
It was a copy-paste from another node I believe so I removed it.

jvillafanez · 2017-03-08T10:13:19Z

I've also seen that the windows prompts aren't translatable... I think that we should make them translatable in order to merge this, so we can change that part at any time. Otherwise we might need to rush to change the implementation if this is merged.

jvillafanez · 2017-03-08T10:14:10Z

I haven't reviewed the tests yet.... 😓

SergioBertolinSG · 2017-03-08T12:43:29Z

When adding new users to custom group there is no autocomplete in the textfield. This will come after this PR or it is a bug here?

SergioBertolinSG · 2017-03-08T12:53:52Z

When editing a member role, just a click on the pencil change member's role. Feels a bit unexpected, I think it should be a dropdown with the options admin or member.

PVince81 · 2017-03-08T13:52:07Z

windows prompts

You mean the native window.prompt ? We need to replace these anyway... I put these here to save some dev time. They definitely need a better replacement. (separately)

PVince81 · 2017-03-08T14:39:40Z

@SergioBertolinSG I've raised them separately:

autocomplete to be added later: Autocomplete for the member field #32
permission change should be a dropdown instead of pencil: Better UI for switching permission #33

PVince81 · 2017-03-08T16:31:27Z

Addressed all review comments and squashed the fixes into the last commit.

Note that I also renamed the unit tests from CustomGroupsCollection to GroupsCollection now.
Originally I had "CustomGroups" as prefix everywhere but it became boring so I renamed the JS files but forgot to rename the test files. This is done here now.

@jvillafanez another check ?

jvillafanez · 2017-03-09T09:49:49Z

You mean the native window.prompt ? We need to replace these anyway... I put these here to save some dev time. They definitely need a better replacement. (separately)

Yes. My point is that I don't know how much time we have to change them. If we make their text translatable, we could release the app. It's ugly, nobody likes it, but it's usable. We can improve the app later. (I'm refering to the windows prompts, not the app itself 😄 )
If we merge it as it is, we'll have to change it before releasing the app because those strings aren't translatable, and we shouldn't release the app under those conditions.

PVince81 · 2017-03-09T10:02:38Z

We definitely shouldn't release the final app with the ugly window prompts.
Raised #36

PVince81 added the 2 - Developing label Dec 14, 2016

PVince81 added this to the 10.0 milestone Dec 14, 2016

PVince81 self-assigned this Dec 14, 2016

PVince81 force-pushed the frontend branch 2 times, most recently from 87a4cad to 2046e8c Compare December 19, 2016 10:12

PVince81 mentioned this pull request Dec 19, 2016

Frontend for custom groups management #5

Closed

13 tasks

PVince81 force-pushed the frontend branch from 2046e8c to bae2027 Compare February 3, 2017 12:52

This was referenced Feb 8, 2017

Restrict proppatch to the proper nodes owncloud/core#27118

Merged

WebdavNode backbone models and collections owncloud/core#27119

Merged

PVince81 force-pushed the frontend branch 5 times, most recently from f12b48d to 61b0149 Compare February 17, 2017 10:14

PVince81 force-pushed the frontend branch from 7a61191 to 843b25f Compare February 23, 2017 13:55

PVince81 mentioned this pull request Feb 24, 2017

Publish notification when adding user #28

Merged

1 task

PVince81 force-pushed the frontend branch 2 times, most recently from 9899c96 to 059007d Compare March 7, 2017 11:39

Improved more modular Makefile

ae6fc41

PVince81 force-pushed the frontend branch from a66c46e to b2d8fe8 Compare March 7, 2017 18:31

PVince81 added 3 - To Review and removed 2 - Developing labels Mar 7, 2017

jvillafanez reviewed Mar 8, 2017

View reviewed changes

Add frontend JS + JS unit tests

a380986

PVince81 force-pushed the frontend branch from b2d8fe8 to a380986 Compare March 8, 2017 16:30

PVince81 mentioned this pull request Mar 8, 2017

Frontend pagination #35

Closed

2 tasks

PVince81 merged commit de7ba54 into master Mar 9, 2017

PVince81 deleted the frontend branch March 9, 2017 10:10

Preliminary frontend work #13

Preliminary frontend work #13

Conversation

PVince81 commented Dec 14, 2016

PVince81 commented Dec 19, 2016 • edited Loading

PVince81 commented Dec 19, 2016 • edited Loading

PVince81 commented Feb 3, 2017

PVince81 commented Feb 8, 2017

PVince81 commented Feb 8, 2017 • edited Loading

PVince81 commented Feb 8, 2017 • edited Loading

PVince81 commented Feb 23, 2017

PVince81 commented Mar 7, 2017

PVince81 commented Mar 7, 2017

PVince81 commented Mar 7, 2017

PVince81 commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Mar 8, 2017

jvillafanez commented Mar 8, 2017

SergioBertolinSG commented Mar 8, 2017

SergioBertolinSG commented Mar 8, 2017

PVince81 commented Mar 8, 2017

PVince81 commented Mar 8, 2017

PVince81 commented Mar 8, 2017

jvillafanez commented Mar 9, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Dec 19, 2016 •

edited

Loading

PVince81 commented Dec 19, 2016 •

edited

Loading

PVince81 commented Feb 8, 2017 •

edited

Loading

PVince81 commented Feb 8, 2017 •

edited

Loading