Bookmarklet handling form in bukuserver ignores entered values

LeXofLeviafan commented 1 year ago

When using the bookmarklet to open a "modal" window with create bookmark form, the Url and Title fields are pre-filled; the user then can modify the form data however he likes before submitting it.

However, regardless of the changes made by user, the only value from his input that gets saved to DB is Tags – the rest are taken from the page data. Worse yet, the user can't even see the Description value until the form is submitted (it's not displayed in the respective field of creation form).

P.S. Also it would make more sense to close the "modal" window on Save/Cancel click, would it not?

LeXofLeviafan commented 1 year ago

…It seems that the cause of the problem is twofold:

the POST data of the /bookmark/new request gets overwritten by URL parameters (retained from the GET request that was used for the form creation)
independently of that, bukudb appears to fetch title & description values from the URL on bookmark creation, when these values are empty
(Is that actually intended? Shouldn't there be a way to disable this behaviour, at least? I don't see any mention of it in the bukuserver readme, and I don't expect such behaviour when submitting a manually-filled form either…)

Edit: Forgot to add – the desciption field is initially empty because it's implemented differently in the bookmarklet: it attempts to read selected text (…which I believe isn't mentioned anywhere except its source), and if there's no text selected it doesn't fall back to default behaviour.

rachmadaniHaryono commented 1 year ago

to summarize

submitted description and title is not added when created record
automated value for description is not shown , when description from bookmarklet is empty
- i also assume this happen to title field

P.S. Also it would make more sense to close the "modal" window on Save/Cancel click, would it not?

yes, but bukuserver use the default form from flask admin so no button to close window

it is nice to have feature if it can close the window after save or cancel but if it ok to not have it

above section is made before i read your second post

bukudb appears to fetch title & description values from the URL on bookmark creation, when these values are empty (Is that actually intended? Shouldn't there be a way to disable this behaviour, at least? I don't see any mention of it in the bukuserver readme, and I don't expect such behaviour when submitting a manually-filled form either…)

bukudb appears to fetch title & description values from the URL on bookmark creation, when these values are empty

that is default bukudb behaviour

Shouldn't there be a way to disable this behaviour, at least?

@jarun , i assume there is, i will check it later

I don't see any mention of it in the bukuserver readme, and I don't expect such behaviour when submitting a manually-filled form either…)

this is actually an issue. there are 2 ways to go about it

disable all auto fill on all record created from bukuserver
let user know that empty title/tags/description will be filled automatically. something like adding text "leave empty to let buku fill it"

i favour option 2 and edit the create bookmark form

However, regardless of the changes made by user, the only value from his input that gets saved to DB is Tags – the rest are taken from the page data.

i'm on bf0f7af and i can't reproduce it , steps:

go to https://github.com/jarun/buku, make sure it is not added to buku yet

select this line

For those who prefer the GUI, bukuserver exposes a browsable front-end on a local web host server.

go to bukuserver last bookmark page

view buku record

title: jarun/buku: Personal mini-web in text
description: For those who prefer the GUI, bukuserver exposes a browsable front-end on a local web host server.
tags:

compare this to add only buku url

Title : GitHub - jarun/buku: Personal mini-web in text
Tags : 
Description : :bookmark: Personal mini-web in text. Contribute to jarun/buku development by creating an account on GitHub.

unrelated i may not be able to test pr / reply issue from friday - sunday,

also i havent work on network test cache yet, so it may have to wait till monday

LeXofLeviafan commented 1 year ago

select this line

See my edit of the previous post

i can't reproduce it

Are you sure you've understood my explanation correctly? Here's the user actions:

open a page which hasn't been added to buku yet
click the bookmarklet
edit every field in the form (e.g. add #foo to Url, replace Title with bar, replace Description with baz)
click any of the save buttons and navigate to the new bookmark
- expected behaviour: manual changes are retained
- actual behaviour: manual changes are discarded (except for Tags)

LeXofLeviafan commented 1 year ago

i assume there is

Buku API has the parameter to control it, but there's no option to use it from the GUI

rachmadaniHaryono commented 1 year ago

i can reproduce the issue with your steps

this is weird one because without edit just like my steps, description data is kept if it is not empty

another weird behavior

open https://github.com/jarun/buku which hasn't been added to buku yet 1.2 select text Personal
click the bookmarklet
replace Description with baz
click any of the save buttons and navigate to the new bookmark

expected behaviour: description should be baz actual behaviour: description is the selected text

unrelated

but there is error when changing between tabs when creating bookmark

127.0.0.1 - - [08/Dec/2022 11:35:03] "GET /bookmark/details/?id=2648&url=%2Fbookmark%2F HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 2548, in __call__
    return self.wsgi_app(environ, start_response)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 2528, in wsgi_app
    response = self.handle_exception(e)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 2525, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 1822, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask_api/app.py", line 106, in handle_user_exception
    raise e
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 1820, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask/app.py", line 1796, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask_admin/base.py", line 69, in inner
    return self._run_view(f, *args, **kwargs)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask_admin/base.py", line 368, in _run_view
    return fn(self, *args, **kwargs)
  File "/home/r3r/.pyenv/versions/buku/lib/python3.10/site-packages/flask_admin/model/base.py", line 2183, in details_view
    model = self.get_one(id)
  File "/mnt/ac54dceb-73a5-4f94-b52c-cb7a426c0f29/Documents/Buku/bukuserver/views.py", line 281, in get_one
    setattr(bm_sns, field.name.lower(), bookmark[field.value])
TypeError: 'NoneType' object is not subscriptable

LeXofLeviafan commented 1 year ago

another weird behavior

I'm not sure how it's different from what I described… Or is the weird part that the edit of non-empty description got reset? I've described what's happening there in the second comment.

there is error when changing between tabs when creating bookmark

How did it happen? The only way I've managed to reproduce it is by pressing "Save and Add Another" then "Save and Continue Editing" (because id got set to -1), which doesn't seem to be how you got it to happen.

P.S. Turns out it's possible to close the tab automatically if it's opened in a popup window (i.e. by the bookmarklet) – if I add this in bookmarks_list.html, it'll close when Save/Cancel is pressed (due to backlinks); and adding it to home.html, statistic.html & tags_list.html as well basically limits the popup functionality to create/edit form & the details page.

Also I've managed to reproduce (more or less) fetch behaviour of buku in the bookmarklet as fallback behaviour (i.e. detecting description from metadata if no text is selected).

LeXofLeviafan commented 1 year ago

by pressing "Save and Add Another" then "Save and Continue Editing" (because id got set to -1)

Note: this happens only when I don't change the URL before saving (add_rec returns -1 if the URL exists in DB?)

Edit: it does according to the source, but is this behaviour documented? I can't seem to find any mention of it anywhere.

rachmadaniHaryono commented 1 year ago

I'm not sure how it's different from what I described… Or is the weird part that the edit of non-empty description got reset? I've described what's happening there in the second comment.

instead of value from bukudb it default to selected text

How did it happen? The only way I've managed to reproduce it is by pressing "Save and Add Another" then "Save and Continue Editing" (because id got set to -1), which doesn't seem to be how you got it to happen.

i think i am mistaken between creating record and editing it

full step

go to edit page on first tab
open new tab and delete that bookmark
on first tab click details button

P.S. Turns out it's possible to close the tab automatically if it's opened in a popup window (i.e. by the bookmarklet) – if I add this in bookmarks_list.html, it'll close when Save/Cancel is pressed (due to backlinks); and adding it to home.html, statistic.html & tags_list.html as well basically limits the popup functionality to create/edit form & the details page.

if it is easy, add it

by pressing "Save and Add Another" then "Save and Continue Editing" (because id got set to -1)
Note: this happens only when I don't change the URL before saving (add_rec returns -1 if the URL exists in DB?)

Edit: it does according to the source, but is this behaviour documented? I can't seem to find any mention of it anywhere.

add_rec returns -1 ... https://github.com/jarun/buku/blob/bddc9385d08a722e6a4f07487068fbd6eda2e8e5/buku#L610

it mention failure and include the case of url already exist on db

that one? or you mean something else

LeXofLeviafan commented 1 year ago

open new tab and delete that bookmark

on first tab click details button

Er, I mean… You just deleted it. The page you're navigating to no longer exists :sweat_smile:

Edit: this condition can be handled by the code – I'll include it in my next commit.

it mention failure and include the case of url already exist on db

When I said "documented", I meant outside of the source – it seems to be a behaviour that would affect UX. Somehow I doubt that the CLI which uses this function would behave differently from it. (…Also the docstring says nothing about disallowing duplicate URLs in database, as far as I can tell. Nor does it mention any other possible reason of failure, for that matter.)

rachmadaniHaryono commented 1 year ago

Er, I mean… You just deleted it. The page you're navigating to no longer exists sweat_smile

Edit: this condition can be handled by the code – I'll include it in my next commit.

it should be 404 instead of NoneType error

When I said "documented", I meant outside of the source – it seems to be a behaviour that would affect UX. Somehow I doubt that the CLI which uses this function would behave differently from it.

(…Also the docstring says nothing about disallowing duplicate URLs in database, as far as I can tell. Nor does it mention any other possible reason of failure, for that matter.)

something like this on example section (between example 1 and 2)

❯ buku --nostdin -a https://github.com/
2648. GitHub: Let’s build from here · GitHub
   > https://github.com/
   + GitHub is where over 94 million developers shape the future of software, together. Contribute to the open source community, manage your Git repositories, review code like a pro, track bugs
     and features, power your CI/CD and DevOps workflows, and secure code before you commit it.

❯ buku --nostdin -a https://github.com/
[ERROR] URL [https://github.com/] already exists at index 2648

The command will add the url to buku. Title and description will be fetched from the site. Buku only keep the unique url and it will raise error if the url already exist

LeXofLeviafan commented 1 year ago

I've committed a fix – I think it covers everything from here? Except for documentation changes and the option to turn off auto-filling empty fields.

It's based off https://github.com/jarun/buku/commit/bf0f7af21a5e91fd49f78bed5b54bb937d4a8f63 so there's little point in making a pull request for it until the other one is merged.

something like this on example section (between example 1 and 2)

:+1:

LeXofLeviafan commented 1 year ago

…Added several more commits – I've put them on a separate branch but I'm thinking of making a pull-request from it directly since it includes d0b86ce anyway (which hasn't made it to a pull-request yet due to being dependent on #638). Originally it was meant as a single commit but it got slightly out of hand so I decided to split it into several for clarity.

It includes a checkbox in create form allowing to turn off fetch functionality, moving of API into a separate file, and a bunch of minor fixes & improvements in bukuserver code (mostly in templates, plus responses code duplication in API). I've also added a LOCALE variable to enable partial l10n support in case someone wants it (there's code that accounts for l10n but no easy way to test it out). …I'll describe the changes more thoroughly when making a pull-request.

LeXofLeviafan commented 1 year ago

something like this on example section

I've created a separate issue for this, to ensure it isn't lost & forgotten.

jarun / buku

Bookmarklet handling form in bukuserver ignores entered values #639