remove modal and vue.js, create guests only by email #149

cortho · 2017-09-13T10:42:43Z

Related issues:
#147, ~~#115~~

I have been testing locally at the frontend and with the integration tests.

What is done in this PR:

moved dynamically created guests.bundle.js to guestshare.js
removed modal window (fixes Remove guest creation modal #147)
~~extended \Files_Sharing\Controller\ShareesController to be able to distinguish between guests and oc users (Guest users cannot be distinguished from regular users in the sharingTabView #115)~~
use constant for default group name
~~show (guest) in autocomplete, when user is a guest (this value will be taken from appconfig's group name)~~
~~do not offer guest user creation in autocomplete when an email is already in use~~
removed vue.js along with composer & co
removed build information from Makefile

Edit: current status

PVince81 · 2017-09-13T14:53:34Z

js/sharedialogview.js

+		this.model = model;
+		this.email = email;
+
+		let self = this;


does let work with IE11 without transpiler ?

and other browsers like latest Safari, etc... I'd prefer to be safe and use var instead

PVince81 · 2017-09-13T14:54:37Z

js/sharedialogview.js

+		}).fail(function (xhr) {
+			let response = JSON.parse(xhr.responseText);
+			let error = response.errorMessages;
+			OCdialogs.alert(


optional: OC.dialogs.alert (OC.dialogs is an alias of the internal OCdialogs)

PVince81 · 2017-09-13T14:55:36Z

js/sharedialogview.js

+			data: attributes,
+			dataType: 'json'
+		}).done(function () {
+			if (self.model)


please add brackets even for a single line

see https://doc.owncloud.org/server/10.0/developer_manual/general/codingguidelines.html#control-structures

PVince81 · 2017-09-13T14:56:15Z

js/sharedialogview.js

+OC.Plugins.register('OC.Share.ShareDialogView', {
+	attach: function (obj) {
+
+		// Override ShareDigalogView


PVince81 · 2017-09-13T14:56:43Z

js/sharedialogview.js

+				function (result) {
+					$loading.addClass('hidden');
+					$loading.removeClass('inlineblock');
+					if (result.ocs.meta.statuscode == 100) {


optional: use "api/v2" for slightly more status code sanity and receive 200 here instead of 100

PVince81 · 2017-09-13T14:57:52Z

js/sharedialogview.js

+ *
+ */
+
+var Guests = {


class is called Guests but the file is called sharedialogview.js ?

PVince81

I don't think this is the right approach.

What if another app also decides to influence the share autocomplete ? They'd both fight for the sharee route.

Also I'm not happy with the huge copy-paste of the share dialog view code. If we ever fix stuff in core there we'd need to recopy again.

How did the old vuejs code do ? From what I remember we added a plugin hook in core to allow an app to inject stuff / results from its own JS class. Did it not work with that approach ?

PVince81 · 2017-09-13T15:05:29Z

I also need to think about it a bit... my gut feeling tells me that doing "do not offer guest user creation in autocomplete when an email is already in use" is not trivial and might need some backend work instead.

PVince81 · 2017-09-13T15:13:46Z

js/sharedialogview.js

+
+		// Override ShareDigalogView
+
+		obj.autocompleteHandler = function (search, response) {


ok now I realize that this is the old existing code that you just moved here... argh... I wish I saw this earlier

actually what I expected from the original dev was to extend the function, not copy paste it.
To extend:

var oldHandler = obj.autocompleteHandler; obj.autocompleteHandler = function(search, response) { return oldHandler.call(obj, search, function() { // add additional entries here (challenge might be to get it sorted properly) // ... response(); }); }

We should probably add more hooks in core for the autocomplete handler to allow injecting results inside the array from separate/different requests so they can be sorted and also the "no users" message would work too.

Trying to think of a way without additional hooks...

Raised here #150

PVince81 · 2017-09-13T15:15:47Z

js/sharedialogview.js

+
+						// Add potential guests to the suggestions
+						if (searchTerm.search("@") !== -1) {
+							groupName = groupName = "(gast)"; 


(gast) would need to be translated: t('guests', '(guest)')

and syntax problem, there are two equals. Change this row to:

groupName += ' ' + t('guests', '(guest)');

PVince81 · 2017-09-13T15:32:27Z

Sorry if my comments might sounds confusing. I commented as soon as I saw the code.
I'm starting to get a fuller picture of what you are trying to achieve.

From what I understand:

ShareesController was overridden to be able to append "($guestGroupName)" for any guest user in the result list
client-side JS is just a copy of the old vue.js plugin registration, the only change is the endpoint that goes to the guest app's sharee endpoint instead of the core one, to reach ShareesController
the client-side autocompleteHandler override is only here to inject the extra "create new guest" entry in case an email address was entered.

I'd like to find a way that removes the need to override/extend the ShareesController.
On the other hand, one might say, if we already copy-pasted so much code for the JS override, why not do the same for the server ? :-/

PVince81 · 2017-09-13T15:35:55Z

Ideal would to be to have the distinction possible in core, we have been discussing this here: owncloud/core#28979

PVince81 · 2017-09-13T15:44:37Z

@cortho I had a chat with @pmaier1 and we agreed that the suffix "($guestGroupName)" is not needed for this version. We'll look into a way for distinguishing users in the share autocomplete directly in core: owncloud/core#28979

Can you remove ShareesController and the custom route ?
Removing the dialog and switching to email should be enough for now.

cortho · 2017-09-14T07:57:27Z

@PVince81 ok. I'll look over this and give feedback/changes ASAP

cortho · 2017-09-14T10:32:21Z

@PVince81 I removed SharesController and we are now extending the autocomplete handler, plus the cosmetic/syntax fixes from above. The PR is much smaller now.

PVince81

Looks much better, thanks a lot!

Needs some careful testing especially for cases where the server response might be empty but we still add the guest entry. And cases where we don't display any result at all.

See comments, some additions are needed to cater for such cases.

PVince81 · 2017-09-14T11:34:02Z

js/guestshare.js

+				username: this.email,
+				email: this.email
+			}
+		}


I suggest running JSLint/JSHint on your JS code. It would tell you to add a semicolon here.

PVince81 · 2017-09-14T11:36:32Z

js/guestshare.js

+					});
+				}
+
+				response(result);


so it did work, that is really awesome !

Did you notice any sort order issue while testing ? From what I see the original handler sorts before calling the callback: https://github.com/owncloud/core/blob/master/core/js/sharedialogview.js#L248. So you might need to sort again here. Since the list is usually not too long it's ok to do it again.

PVince81 · 2017-09-14T11:37:03Z

js/guestshare.js

+
+		    return oldHandler.call(obj, search, function(result) {
+
+				var searchTerm = search.term.trim();


please check if result is defined, if not, call response(). See https://github.com/owncloud/core/blob/master/core/js/sharedialogview.js#L269 for an example when it can happen.

cortho · 2017-09-14T14:06:12Z

@PVince81 Thank you for your good review. It looks like we're done then.

PVince81 · 2017-09-14T15:54:23Z

hmmm weird

± % git checkout -b cortho-new_sharetab_behavior master
git pull https://github.com/cortho/guests.git new_sharetab_behavior

Switched to a new branch 'cortho-new_sharetab_behavior'
From https://github.com/cortho/guests
 * branch            new_sharetab_behavior -> FETCH_HEAD
fatal: refusing to merge unrelated histories

PVince81 · 2017-09-14T15:54:46Z

wrong repo 🤦‍♂️

PVince81 · 2017-09-14T15:55:15Z

@cortho thanks a lot, I'll test it a bit

PVince81 · 2017-09-14T16:04:26Z

TEST: share with email, create guest, account works
TEST: share with existing guest by entering email address
BUG: if I type in the email address of an existing user, the user appears both as "Add guest" and as existing user

@cortho to fix this, I suggest that in your autocompleteHandler, iterate over the results and check if there is any result with the same email address. If yes, then don't add "Add guests".

BUG: disable federated sharing (settings/Sharing, untick both under "Federated Cloud Sharing) then share with a new email address like "mail@example.com" (make sure there is no other result). The dropdown does not appear.

Not sure how to fix this. The former autocomplete handler had no results in the set and decided to not display the dropdown, so adding an entry afterwards doesn't properly re-set it.

@cortho would be good if you could fix at least the first issue above. The second one might need dirty hacks, not sure if it will work.

PVince81 · 2017-09-14T16:06:25Z

js/guestshare.js

+					});
+				}
+
+				if (result !== undefined) {


please move this check at the begining of the function. Because if result is undefined, the result.push above will fail hard

PVince81 · 2017-09-14T16:10:24Z

@cortho another trick to try: instead of adding "Add guest" result inside the overridden handler, add it to the results returned by the server before calling oldAutocompleteHandler. Might help solving the second issue and will also resolve the double sorting issue.

PVince81 · 2017-09-14T20:05:22Z

I need to finish this, merging for now. Almost there.

PVince81 · 2017-09-14T20:05:42Z

the travis tests fail for some strange reason, need to investigate separately

PVince81 · 2017-09-14T20:07:51Z

Follow up here: #151

felixboehm · 2017-10-04T12:33:19Z

This PR removed vue.js, which we agreed on very clear to use as FE framework for this app.
Also the functionality is completely broken, app whitelist is not working.

@cortho

felixboehm · 2017-10-04T12:36:01Z

Question here about vue.js #147 (comment)

IMHO: Even without modal, the vue.js is still needed. So do not remove it.

remove modal and vue.js, create guests only by email

92220f8

cortho added the 3 - To review label Sep 13, 2017

cortho added this to the development milestone Sep 13, 2017

cortho self-assigned this Sep 13, 2017

This was referenced Sep 13, 2017

Guest users cannot be distinguished from regular users in the sharingTabView #115

Open

Remove guest creation modal #147

Closed

cortho requested a review from PVince81 September 13, 2017 12:08

PVince81 reviewed Sep 13, 2017

View reviewed changes

js/sharedialogview.js Outdated

*

*/

var Guests = {

Copy link

Contributor

PVince81 Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class is called Guests but the file is called sharedialogview.js ?

PVince81 suggested changes Sep 13, 2017

View reviewed changes

PVince81 reviewed Sep 13, 2017

View reviewed changes

PVince81 mentioned this pull request Sep 13, 2017

Make duplicate sharee autocomplete results easier to distinguish owncloud/core#28979

Closed

remove ShareesController, extend ShareDialogView

ec2756d

PVince81 suggested changes Sep 14, 2017

View reviewed changes

handle undefined result, sort result

8f0f775

PVince81 reviewed Sep 14, 2017

View reviewed changes

PVince81 merged commit 3d80a81 into owncloud:master Sep 14, 2017

PVince81 mentioned this pull request Sep 14, 2017

Fix guest autocomplete addition #151

Merged

cortho deleted the new_sharetab_behavior branch September 15, 2017 09:31

felixboehm added the p1-urgent label Oct 4, 2017

PVince81 mentioned this pull request Oct 9, 2017

Get rid of major copy-paste of core sharedialogview's autocompleteHandler #150

Closed

cortho mentioned this pull request Oct 9, 2017

Enter key must save #56

Closed

davitol mentioned this pull request Oct 26, 2017

0.5 Release Plan #176

Closed

21 tasks


		// Override ShareDigalogView

		obj.autocompleteHandler = function (search, response) {


		return oldHandler.call(obj, search, function(result) {

		var searchTerm = search.term.trim();

remove modal and vue.js, create guests only by email #149

remove modal and vue.js, create guests only by email #149

Conversation

cortho commented Sep 13, 2017 • edited Loading

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

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Sep 13, 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

PVince81 Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

PVince81 commented Sep 13, 2017

PVince81 commented Sep 13, 2017

PVince81 commented Sep 13, 2017

cortho commented Sep 14, 2017

cortho commented Sep 14, 2017

PVince81 left a 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

cortho commented Sep 14, 2017

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

Choose a reason for hiding this comment

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

PVince81 commented Sep 14, 2017

felixboehm commented Oct 4, 2017

felixboehm commented Oct 4, 2017

cortho commented Sep 13, 2017 •

edited

Loading

PVince81 Sep 13, 2017 •

edited

Loading