Proposal for favorite sharing by public link

[paulschwoerer]

Hey guys

As discussed in issue #70, I propose these changes to be able to share favorites by public link (per category).
It's currently possible as a logged in user to share/unshare a category. The shared category can be viewed using the public link.

Let me know what you think of the current design and functionality.

Cheers!

[jancborchardt]

Cool stuff! Will try and test soon :)

As for the roadmap, let’s try and keep pull requests small and contained! :) The most important thing is that sharing a link simply works, I’d even say the first thing could be just read-only.

Because the next step is a toggle for read-only / edit for the share link, and that’s already a second step which is better to do in a follow-up pull request.

[paulschwoerer]

I’d even say the first thing could be just read-only.
Because the next step is a toggle for read-only / edit for the share link, and that’s already a second step which is better to do in a follow-up pull request.

I agree, the next step would then be for me to make a public link read-only by default and implement constraints accordingly in the backend.

[julien-nc]

Wow this is amazing!

Several remarks:

• your Migration script is not triggered because the app version didn't change
• the public link address is partial. there is no https://hostname (you can prepend window.location.origin to it)
• the public page does not load because it fails getting js/maps.js which is not there (maybe you forgot to include it in the repo)

Looking forward to use it! Again: amazing!

[paulschwoerer]

@eneiluj Thanks for the enthusiasm :)

• Mmh I need to look into that and bump the app version, thanks
• I thought there might be another way to do it using nextclouds methods like OC.generateUrl(), but window.location.origin will do 👍
• Currently the frontend should be compiled on the fly using npm run watch. For distribution, a maps.js file should be created using npm run build. Otherwise dev dependencies and stuff will unnecessarily included. I might be able to attach a zip here with the built code.

Cheers!

[julien-nc]

Sorry I'm not familiar with all that. Could you explain what needs to be done to try your branch?

[julien-nc]

Oh and you can use OC.generateUrl() but you still need to prepend protocol://hostname

[paulschwoerer]

@eneiluj You might be able to use this to deploy a nextcloud development environment locally. Just change the file .gitmodules to:

[submodule "maps/nextcloud-docker"]
	path = maps/nextcloud-docker
	url = https://github.com/nextcloud/docker.git
[submodule "maps/src/maps"]
	path = maps/src/maps
	url = https://github.com/nextcloud/maps.git
        branch = issue-70-share-favorite-locations

I might attach a zip file with a built version soon

[paulschwoerer]

@jancborchardt @eneiluj When changing a favourite's category using the editing endpoint, it's possible to create a situation where there will be no favourites left with say category A. If the user previously shared category A, this share is still active but no favourites are associated with it.

I currently have two possibilities in mind to correct this behaviour:

1. On every edit where a category is changed check the database if there are favorites with the category left. If not, delete any associated shares. This might be bad for performance as the check needs to be done whenever a favourite's category is edited.
2. Do nothing on edit, but check the database when a share is retrieved. If no favourites with the shared category are left, delete the share and return a 404.

What are your opinions on this? Do you have other ideas?

[jancborchardt]

@paulschwoerer keeping in mind that editing by others will be possible in the future, having 0 elements in the share should not equate to the share being removed. :) Because for example:

• You could share an empty favorites list for others to add stuff
• You could delete current entries and then add new ones, only for you to discover you have to recreate the share (which is strange)

Hence I’d say the share should still work fine, just simply in the part where the favorites are displayed it could display an emptycontent element (similar to an empty folder in Files):

icon-favorite
**No favorites yet
Add some to this list

If you don’t have editing rights, the text in the second row could instead be "This is a read-only list".

[paulschwoerer]

@jancborchardt Great thoughts! I agree on having empty categories makes sense. Would it then as a consequence make sense for a category to be represented by a standalone database entity, instead of a field in the favorite table?

[jancborchardt]

Would it then as a consequence make sense for a category to be represented by a standalone database entity, instead of a field in the favorite table?

I’m going to hand over implementation-specific questions to @eneiluj @tacruc @aszlig and others :)

[tacruc]

@paulschwoerer nice work 👍

I agree on having empty categories makes sense. Would it then as a consequence make sense for a category to be represented by a standalone database entity, instead of a field in the favorite table?

I guess this makes sense. Can the favorite shares table then be merged
and droped, or should we keep them separated @eneiluj @paulschwoerer ?

[paulschwoerer]

nice work

Thanks :)

@tacruc I think we should't drop it, as a user might want to share a favourite category with multiple links (one can edit, the other not, ...) and to other users on the server in the future. For this we'd need a separate table for shares.

[anaqreon]

I'm late to the game here, but if it helps other who want to test this, I pushed a new branch issue-70 to https://github.com/anaqreon/nextcloud-maps-dev/tree/issue-70 that should allow you to

git clone --recursive --branch issue-70 https://github.com/anaqreon/nextcloud-maps-dev.git
./nextcloud-maps-dev/maps/init.sh

to get a working instance of @paulschwoerer 's branch (as of 678377a at least). The only thing I verified so far is that the public share link indeed shows shared favorites.
(Apologies, the close pull request action was a mistake.)

[anaqreon]

The Docker environment I referenced above actually works now; I had not noticed a problem with the git submodule initialization when I posted yesterday. Testers should not be alarmed by the number of "errors" that result from the npm run watch command. Most of them appear to be generated by prettier/prettier and are complaints about trailing commas in lists and other code-style related issues.

[jancborchardt]

@paulschwoerer sorry, am quite busy with Nextcloud 18 work at the moment. Could you post some screenshots of the main parts? That would help a lot with design review. :)

[paulschwoerer]

@jancborchardt Here you go! Screens of the sharing dialogue. I'll upload some of the public share page soon

[paulschwoerer]

@jancborchardt And some screens of the public part

Overview

Map Click Popup

Clustering

[jancborchardt]

Great work! :) Simple and straightforward, love it!

Quick review:

• The sharing entry should be a popover, like for example in Talk for link sharing. :) See the actions component
• The wording can just be "Share link". Using the word "public" sounds like it is going to be uploaded to a public directory, which is not the case.
• Instead of showing the link, the second entry in the popover (when the box is checked) should be "Copy link" which simply copies the link to the clipboard. Needs tooltip for feedback

Nice on the share page! Some details:

• The footer shouldn’t take away from the size of the map. Can you put it on the bottom of the navigation instead?
• The "shared by paul" subline in the header should be the same color as the first row
• In the navigation, the list looks a bit bland without any icons, and with the entries indented. With all of them having the icon-star (the grey one so it’s not too extreme) it would be much nicer. (In the future it can be icons for the types of establishments, like restaurant or café)
• The zoom controls are on the top left? Since the logged in map has it on the bottom right, it’s best to follow the same system. :) (Also if we ever introduce search there for example.)

Map click popup and clustering look like in the app when logged in, right? :)

[anaqreon]

I agree that this is excellent work. It is already providing a very useful feature even in this first iteration.

I don't have anything to add to @jancborchardt 's comments except something odd I noticed in the display when tabbing through links. This quirk may be an issue with Maps in general and not this specific pull request; I'm not sure yet. What I find is that as I press the tab key after revealing the share link (to conveniently copy the URL) is that the navigation pane gets shifted over and doesn't recover until many tab key presses later. See the screenshots below.

Click share icon:

Press tab key:

Press tab key again:

[paulschwoerer]

The sharing entry should be a popover, like for example in Talk for link sharing

Instead of showing the link, the second entry in the popover (when the box is checked) should be "Copy link" which simply copies the link to the clipboard. Needs tooltip for feedback

@jancborchardt Talk seems to be using a different approach, where a public sharing token is generated when a room is created. It can then be copied from the popover menu. I'm currently only generating share tokens once the user checks the sharing checkbox. This has two main benefits IMO:

1. Access can be revoked by deleting the token.
2. later on, multiple shares can be created (e.g. one editable and pw protected, one read-only)

We could use something like this for now:

[paulschwoerer]

Ready to merge :)

[julien-nc]

@paulschwoerer Great. I didn't realize it was such a big PR. I'll have more time on the weekend.

It also works with npm run build which is triggered by the main makefile target. I guess the watch script is more verbose.

[paulschwoerer]

@eneiluj Exactly, watch will generate a development build and rebuild when files are changed. build will generate a production build.

[julien-nc]

Ok nice about the watch script. Useful stuff!

I'm sorry I didn't get to review this PR yet. I'm focusing on the next small release. I don't forget this though 😁.

[anaqreon]

Finally getting back to looking at this again, and I took the opportunity to improve the development environment repo as well. Hopefully it can help others review these kinds of pull requests more easily and robustly. If you git clone the latest version of nextcloud-maps-dev and run

./maps/deploy issue-70-share-favorite-locations nextcloud:18.0-rc

you should get a fully functioning Nextcloud instance running locally via Docker, where the Nextcloud Docker image used is 18.0-rc and the Maps dev branch is issue-70-share-favorite-locations. The big improvement here is that the files you need to edit to rapidly iterate code while developing are stored in ./maps/www and your local system user owns the files (no more pesky www-data ownership issues!), so you can conveniently change files in your favorite editor and refresh your browser to see the changes immediately. Read the Readme for more details.

Hopefully this will provide enough flexibility and simplicity to encourage more aspiring Maps developers. (Maybe it could even be a time-saver for veterans like @eneiluj).

As for the pull request itself, it all appears to work smoothly and in an interface consistent with other Nextcloud apps. I did receive several warnings during the make process, which I posted in a gist for reference: https://gist.github.com/anaqreon/e38b963631cbac130afc112abcdcb4fd

[paulschwoerer]

@anaqreon An updated development environment sounds neat. I'll check it soon :)

As for the warnings, they don't seem to be a pressing issue, but should be addressed eventually. Not all of them are related to the changes made in this branch.

[julien-nc]

Impressive work. Nicely done.

I didn't really dig into the vue.js part... Still not operational in that area.

Most of my comments are just details about code formatting. Other than that, fine by me. Thanks a lot.

Nice that you already wrote more advanced features. Do you feel most of what's commented/disabled like password-protected share or edition 🚀 is more or less ready? I mean did you disable them to progressively introduce features or is it still not solid?

There is still a little conflict. Just keep my new replace and your new function parameter and that's it!

[paulschwoerer]

Just to give you guys an update: I might be able to put some work into the requested changes this week

[jancborchardt]

Forgot to give my 👍 design-wise. :) So just the code feedback from @eneiluj outstanding.

[paulschwoerer]

@eneiluj I addressed your changes. Unfortunately there are still a lot of formatting changes compared to master. I have to be more careful next time :( If you're fine with the changes, feel free to merge :)

[paulschwoerer]

Nice that you already wrote more advanced features. Do you feel most of what's commented/disabled like password-protected share or edition rocket is more or less ready? I mean did you disable them to progressively introduce features or is it still not solid?

Some features are partially implemented, however this was commented out quite a while ago, so not sure how functional it is.

[julien-nc]

Thanks for the changes. There was still a problem making it impossible to remove shared accesses. I fixed the SQL query. This new feature is just brilliant and nicely done! Thanks again. 🚀

[paulschwoerer]

It was a pleasure, maybe I'll have time to add features in the future :)

[KuenzelIT]

Is this feature going to be released soon? It would be awesome to be able to share locations.

[jancborchardt]

@tacruc @eneiluj considering all the fixes it would probably be good to do another release? :)

[TheRaven500]

Wow, what a nice feature! Unfortunately i am unable to compile it. So when will it be released? Can't wait longer!!! :-)

[jancborchardt]

@TheRaven500 what are the compile errors? The instructions at https://github.com/nextcloud/maps/#-development-setup should work :)

[TheRaven500]

Ok, i have now successfully built from git, but something is wrong because favorites are not working. During the build process i got errors about "public-favorite-share.js" and "script.js" saying me they are to big. Adding favorites is possible, but if i click something else in my cloud and click back they are gone. Sharing is also not possible. For me it looks like something is write protected (wrong permissions?). Is there an error log or something like that?

[paulschwoerer]

Error logs are located in /your/data/dir/nextcloud.log

[TheRaven500]

Thx, i had success with building! I think the trick was to add "sudo -u www-data" to set the right permissions. It is a very nice app by the way! I really love it! Please keep on the good work! I can imaging that there are a lot of features to implement (changing the icons, sharing routes, writable external shares or adding photos to a point for example). Thank you very much for your work!

[Flight777]

Is there any update on this feature? :D

[TheRaven500]

A good question! I also wait for new features! :- )
Changing the color of the symbols for example would be very nice!