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

v3.0.0 beta #965

Closed
marcelklehr opened this issue Apr 9, 2020 · 47 comments
Closed

v3.0.0 beta #965

marcelklehr opened this issue Apr 9, 2020 · 47 comments

Comments

@marcelklehr
Copy link
Member

marcelklehr commented Apr 9, 2020

Hello there,

I'm happy to announce the first beta release of bookmarks v3.0.0, which most notably includes sharing among a few other goodies. (Update: You can only share folders: Click on folder 'details' and then go to the 'Sharing' tab)

I would be glad to hear your feedback after trying it out in this thread as well as possible bugs in new issues. Don't forget to backup your database before trying this out!

Requirements are:

  • PHP v7.2+
  • PHP extensions mbstring and intl
  • Nextcloud v17+

Changes in this release are:

  • dac2cc7 UI: Selection: Implement "select all" and "cancel selection"
  • cbdcbf1 Better document how the screeenly api url looks like
  • 84e889a Move UI: Automatically open current folder
  • 63ef98d Implement bookmark counting endpoints and UI indicators
  • 18df1c1 Fix bookmark creation: Assign folders correctly
  • aa0963c Drop tables on uninstall
  • 796d6c8 Fix import and export
  • 93e2df6 Allow editing URLs
  • 1ad192c Implement a full children endpoint
  • 79b2922 Breadcrumbs: -AddBookmark button +Add Folder/Bookmark dropdown
  • b543fab Don't enable scraping by default
  • 6d656d3 Drop libgmp dependency
  • 768f311 Implement hash caching
  • Implement sharing and public links
  • Major backend refactoring

Update: Here's the link to beta 2: https://github.com/nextcloud/bookmarks/releases/tag/v3.0.0-beta.2

Cheers! 👾

@sunjam
Copy link

sunjam commented Apr 9, 2020

Opened a topic linking here on the forum.

@sunjam
Copy link

sunjam commented Apr 16, 2020

v3.0.0-1 testing on NextcloudPi 18.0.3 stable. Firefox 74.0 (64-bit)

  • imported 1337 😎 bookmarks to InstanceB
  • Folder structure imported
  • Tags imported
    Screenshot_20200416_110659
  • Where are Untagged bookmarks?
    I do not see any Untagged bookmarks, of which I have plenty on InstanceA with regular bookmarks release. Has a tag been added to all of these bookmarks?
    No bookmarks here
    Try changing your query or add some using the button on the left.
  • no folder icons displayed in listview
  • bookmarks icons displayed in listviewincluding within folders
  • double importing will create duplicate folders
    Screenshot_20200416_112926
  • no feedback or way to know an import is taking place.
    I realize I'm importing a lot of bookmarks, but the user just has to trust that their bookmarks are being imported without visual indication of what is happening.
  • recent bookmarks are now displayed in a new order. Just an fyi
  • when no server limit is set: 1337 bookmarks of 0 available
    Might make more sense as 1337 bookmarks available. reference
  • bookmarks (...) menu not visible within bookmark folders on a 1920x1080 display and requires the scrollbar to access.
    Screenshot_20200416_113930
  • Unsure of where the new share option lives
    Check selecting an individual bookmark, tried (...) menu, details menu. Not sure.
  • open Details Menu will block access to selection options & switch view icon until closed
    Screenshot_20200416_120603

@marcelklehr

This comment has been minimized.

@gibelium
Copy link

gibelium commented Apr 17, 2020

Thank you very much for your work @marcelklehr. I did some initial tests with beta-1.

Environment

NC: 18.0.3
bookmarks: 3.0.0-beta.1

Working

  • create bookmark
  • delete bookmark
  • create folder
  • delete folder
  • move bookmark
  • move folder

Bugs

  • Delete All Bookmarks
    • Displayed error: "Request failed with status code 500"
    • Log entry: Exception: Call to undefined method OCA\Bookmarks\Db\FolderMapper::deleteAll()
  • 'Untagged' always empty
  • Table view does sometimes not shrink when details pane is opened that the context menu is no longer accessible (see: v3.0.0 beta #965 (comment))
  • Sharing folder is not yet possible
    • no results even when entering an existing user or group name
    • spinner spins forever
  • Import bookmarks
    • duplicates folders but not the bookmarks. At least the number of stored bookmarks stays the same after second import
    • page is not refreshed when import finished but needs to be refreshed manually
    • Should we import bookmarks to a dedicated sub folder (e.g. Imported Bookmarks)?
  • Sorting:
    • Bookmarks disappear when changing sorting
    • Page often displays 'No bookmarks here' after changing sorting

Nice-to-Have

If helpful I can take screenshots and/or videos...

@marcelklehr
Copy link
Member Author

marcelklehr commented Apr 17, 2020

Regarding bookmarks Import: I've fixed the UI feedback issue.

duplicates folders but not the bookmarks. At least the number of stored bookmarks stays the same after second import

This is intentional, as the bookmarks app does not allow duplicates. Instead bookmarks can be in multiple folders at once. 🤯

Should we import bookmarks to a dedicated sub folder (e.g. Imported Bookmarks)?

I've thought about that but wasn't sure, what would be most expected. What do you think? Adding imports to a new folder is quite simple technically. We could even import into the currently active folder.

@marcelklehr

This comment has been minimized.

@gibelium
Copy link

This is intentional, as the bookmarks app does not allow duplicates. Instead bookmarks can be in multiple folders at once. 🤯

I'm totally fine ending up without duplicates after a second import. :-)
I think importing the same bookmarks twice is a more accidental use case...

I would import bookmarks into a new folder. When I'm right this is the way like Google Chrome and Firefox handle imports to, so people are used to it and you do not mess up your existing bookmarks.

@gibelium

This comment has been minimized.

@marcelklehr

This comment has been minimized.

@gibelium

This comment has been minimized.

@marcelklehr

This comment has been minimized.

@marcelklehr
Copy link
Member Author

@sunjam recent bookmarks are now displayed in a new order. Just an fyi

How has the order changed? 🤔

@gibelium
Copy link

@marcelklehr, will you be able to provide another build with your latest changes? I can then do some more manual tests during the weekend...

Is there already a plan when to release 3.0.0 final to app store?

@sunjam
Copy link

sunjam commented Apr 17, 2020

@marcelklehr, will you be able to provide another build with your latest changes?

Let us know if you want any specific things tested. 👍

@marcelklehr
Copy link
Member Author

marcelklehr commented Apr 17, 2020

Thank you for taking on the acceptance testing! ❤️

I have not planned a fixed date for the release. It's ready when it's ready :) I will not introduce new features into the new release anymore, though.

will you be able to provide another build with your latest changes?

I'll publish another beta then :)

The main purpose of the beta phase is finding regressions stemming from the refactoring, of which you have caught quite a few already 👌, as well as testing public and private sharing.

The following issues I could not reproduce, so far:

  • Table view does sometimes not shrink when details pane is opened that the context menu is no longer accessible
  • no folder icons displayed in listview
  • bookmarks (...) menu not visible within bookmark folders on a 1920x1080 display and requires the scrollbar to access.

Details on these are appreciated.

The following issues are 'wontfix' (at least for this release; feel free to open separate issue for these):

  • open Details Menu will block access to selection options & switch view icon until closed
  • Export bookmarks:
    • adjust file name
    • choose folder where the export file should be stored
  • Hover over a bookmark name could display the URL instead of the name
  • Allow accessing the context menu with a right-click when Right-Click App is enabled
  • Selection:
    • Allow (multi-)selection for folders and moving/deleting the multi-selection
    • Within the selection context menu (top-right) allow 'select all in folder' beside 'select all visible'

@gibelium
Copy link

gibelium commented Apr 17, 2020

Thank you for your work, @marcelklehr!!!

I will create new issues for the nice-to-have ones mentioned by me...
Regarding the table-shrink issue I will record a short video tomorrow...

@sunjam

This comment has been minimized.

@gibelium

This comment has been minimized.

@gibelium
Copy link

gibelium commented Apr 18, 2020

Delete All Bookmarks

  • Basically works well
  • As this can be a long running task we should think about a visual feedback (e.g. progress bar modal)
  • I got the following log entry after deleting all (about 1900) bookmarks
TypeError: Argument 3 passed to OC\AppFramework\Middleware\MiddlewareDispatcher::beforeOutput() must be of the type string, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 123
{"reqId":"RI4FEc3WWBkZFicqYbHb","level":3,"time":"2020-04-18T07:08:13+00:00","remoteAddr":"127.0.0.1","user":"ncadmin","app":"index","method":"GET","url":"/nextcloud/apps/bookmarks/bookmark/3999/favicon","message":{"Exception":"TypeError","Message":"Argument 3 passed to OC\\AppFramework\\Middleware\\MiddlewareDispatcher::beforeOutput() must be of the type string, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 123","Code":0,"Trace":[{"file":"/var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php","line":123,"function":"beforeOutput","class":"OC\\AppFramework\\Middleware\\MiddlewareDispatcher","type":"->"},{"file":"/var/www/nextcloud/lib/private/AppFramework/App.php","line":125,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud/lib/private/AppFramework/Routing/RouteActionHandler.php","line":47,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"function":"__invoke","class":"OC\\AppFramework\\Routing\\RouteActionHandler","type":"->"},{"file":"/var/www/nextcloud/lib/private/Route/Router.php","line":299,"function":"call_user_func"},{"file":"/var/www/nextcloud/lib/base.php","line":1008,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud/index.php","line":38,"function":"handleRequest","class":"OC","type":"::"}],"File":"/var/www/nextcloud/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php","Line":159,"CustomMessage":"--"},"userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:75.0) Gecko/20100101 Firefox/75.0","version":"18.0.3.0","id":"5e9aa75d79d29"}

@gibelium
Copy link

gibelium commented Apr 18, 2020

Import Bookmarks

  • The name of the imported bookmarks folder could be enhanced including filename and timestamp to be able to distinguish between the several imports
  • Import allows all kinds of files and always confirms with "Import successful"
    • Can we restrict the file chooser to allow only HTML files?
    • Possible feedback for imports:
      • Not supported file format
      • Empty bookmarks file / No bookmarks found
      • Successfully imported x of x bookmarks
      • others?
  • Spinner within the import button may be overlooked
    • Can we open a progress bar modal instead or additionally?

@gibelium

This comment has been minimized.

@gibelium
Copy link

gibelium commented Apr 18, 2020

Shared bookmarks

I shared a single bookmarks folder to another user with enabled options to edit and share the bookmarks. The shared bookmarks are available for the user but the number of bookmarks is zero and the untagged bookmarks are empty although none of the bookmarks is tagged.

  • Should the bookmarks shared with me be counted in 'All Bookmarks'?
  • Should the non-tagged but editable bookmarks shared with me be shown in 'Untagged'?
  • Should we add a category for shared bookmarks next to All, Recent and Untagged bookmarks?
  • Should we allow to unshare a folder as the user the folder was shared with? I can delete a folder that was shared with me but this does not remove the share entry for the one who shared the folder.

shared-bookmarks

@gibelium

This comment has been minimized.

@gibelium

This comment has been minimized.

@marcelklehr

This comment has been minimized.

@marcelklehr
Copy link
Member Author

marcelklehr commented Apr 19, 2020

@gibelium Spinner within the import button may be overlooked

I agree, but a new UI component will not make it into this release :)

@gibelium The name of the imported bookmarks folder could be enhanced including filename and timestamp to be able to distinguish between the several imports

Agreed. => Next release :)

@gibelium Should the bookmarks shared with me be counted in 'All Bookmarks'?

This is deliberate as shared bookmarks are owned by a different user and thus do not count towards the usage limit, if one is set.

@gibelium Should the non-tagged but editable bookmarks shared with me be shown in 'Untagged'?

This is deliberate so far, as there is no simple way to query all shared bookmarks recursively, yet.

@gibelium Should we add a category for shared bookmarks next to All, Recent and Untagged bookmarks?

I like that! This would be possible if we only display the shared folders. => Next release.

@sunjam
Copy link

sunjam commented Apr 19, 2020

Nice to Have's

  • Apply a tag Shared by URL to all bookmarks shared by url.

  • When shared book url is accessed, display this in the Activity app. Link to API info.

➕ You have shared a bookmark via url: Learning Python
✔️ Bookmark accessed via public link: Learning Python

@marcelklehr
Copy link
Member Author

marcelklehr commented Apr 19, 2020

@sunjam Apply a tag Shared by URL to all bookmarks shared by url.

I thought about that, too. What would be your goal with that?

@sunjam
Copy link

sunjam commented Apr 19, 2020

@sunjam Apply a tag Shared by URL to all bookmarks shared by url.

I thought about that, too. What would be your goal with that?

It would be the easiest way I can think of to track which bookmarks I've shared besides the Shared category idea. Plus, tags would be useful for future expansion of sharing, presuming we one day support system tags for Flow automation with Bookmarks. Might also be possible to reflect different types of sharing: users/groups vs public url.

Obviously, this is quite presumptive, but you'll find Bookmark integration via system tags was originally proposed by this server tag discussion some time ago.

@sunjam
Copy link

sunjam commented Apr 19, 2020

Crazy example:

I've shared a folder of bookmarks. Any bookmark with Shared to marcelklehr tag is automatically added. Now, every time I add or remove this tag I can change the bookmarks within this shared folder.

This could also be a way to move towards webhooks #900. Also, see this nextcloud dev request on Deck app tag integration with Flow app from 6 hours ago.

@marcelklehr

This comment has been minimized.

@marcelklehr
Copy link
Member Author

marcelklehr commented Apr 19, 2020

I've deliberately left out sharing by tag, as during the design phase (wow, that was 3 years ago, I think) we felt there were a lot of edge cases and possibly unexpected/non-intuitive behavior that would be hard to get right. Let's not get ahead of ourselves. This is already quite a big release, so I would rather get the current feature set ready before packing in any more features. I appreciate your creativity for future features and how a bright future could look and I'm happy to see issues for all of these, even if I can't promise anything :)

I'm also excited about integrating with Flow, btw. There's already an issue somewhere about that.

@sunjam
Copy link

sunjam commented Apr 19, 2020

@marcelklehr You are totally amazing to have gotten sharing worked out!! It is a Herculean effort that should be lauded! It is truly awesome. Wow. Thank you.

Do you want any other particular aspects of sharing tested for beta2?

@marcelklehr
Copy link
Member Author

@gibelium I create a new bookmark in the shared folder and then move it to the/my root folder.

Ah, moving bookmarks across user borders wasn't working correctly. Nice catch! Fixed.

@gibelium Should we allow to unshare a folder as the user the folder was shared with? I can delete a folder that was shared with me but this does not remove the share entry for the one who shared the folder.

I think ideally, the sharee can remove (read: hide) the shared folder from their view and have a way to get it back again, while only the owner can actually remove permissions. This is not yet implemented, however. My reasoning is that it's possible to share with groups and you shouldn't be able to remove the share for the whole group by deleting it as a sharee. so => next release :D

@gibelium Cancellation of 'New bookmark' operation should be possible. Cancellation of 'New folder' operation should be possible.

heh, the buttons that show these forms are technically toggles, so clicking them again will hide the respective form, but I guess nobody will figure that out 😄 I'll work on this.

@marcelklehr
Copy link
Member Author

@sunjam Do you want any other particular aspects of sharing tested for beta2?

Something I haven't tested as of yet, is how receiving a share, especially one that you cannot edit, plays with floccus syncing.

@marcelklehr

This comment has been minimized.

@gibelium

This comment has been minimized.

@sunjam

This comment has been minimized.

@marcelklehr
Copy link
Member Author

@gibelium This is what I could come up with: https://github.com/nextcloud/bookmarks/wiki/Acceptance-testing

@sunjam When sharing a single bookmark, the link proceeds to share every bookmark I have in the app.

Does that only happen in NC19?

@marcelklehr

This comment has been minimized.

@gibelium

This comment has been minimized.

@sunjam

This comment has been minimized.

@marcelklehr

This comment has been minimized.

@gibelium
Copy link

gibelium commented Apr 28, 2020

I finally started testing. Please find the results of the first session.

The two issues I find so far are:

  1. Import does not work at all
  2. Moving bookmarks into the same folder they are already in seems to delete the bookmarks. At least they vanish... :-)

Environment

Ubuntu 19.10
Google Chrome: 81.0.4044.129
Nextcloud: 19.0.0-beta.5
Bookmarks: 3.0.0-beta.3

Tests

  • Migrates all data correctly from old version

    • ERROR: After migration one bookmark is references twice. It is still in the folder it should be but also in the root folder.
      migration-two-references-to-the-same-bookmark
  • Import bookmarks

    • ERROR: Argument 1 passed to OCA\Bookmarks\Service\FolderService::importFile() must be of the type string, null given, called in /var/www/nextcloud/apps/bookmarks/lib/Controller/BookmarkController.php on line 636
  • Export bookmarks

  • Create a bookmark

    • Edit URL
    • Edit tags
    • Edit notes
    • Edit title
  • Create a folder

    • Edit title
  • Delete folder

  • Move bookmark

    • ERROR: Deletes bookmark when moving it into the folder it is already in
  • Move folder

  • Select multiple bookmarks

    • Delete selection
    • Move selection
      • ERROR: Deletes bookmarks when moving them into the folder they are already in
        move-selected-bookmarks-to-folder-they-are-already-in
  • Filter by recent bookmarks

  • Filter by single tag

  • Filter by multiple tags

  • Filter untagged

  • Add new tag

  • Rename tag

@marcelklehr
Copy link
Member Author

Awesome, we're getting close 🎉

@gibelium ERROR: After migration one bookmark is references twice. It is still in the folder it should be but also in the root folder.

I can't seem to reproduce this. Bookmarks unexpectedly turning up in the root folder has been an issue in the past, though, so I think it's likely that this happened before the migration?

@gibelium
Copy link

I can't seem to reproduce this. Bookmarks unexpectedly turning up in the root folder has been an issue in the past, though, so I think it's likely that this happened before the migration?

Well, I gave the migration test a second try and I also cannot reproduce the case after I cross checked that in the old version the bookmarks were in the correct folders. So yes, it could be that it was a failure in the old version.

@marcelklehr
Copy link
Member Author

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants