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

Bookmarklet handling form in bukuserver ignores entered values #639

Closed
LeXofLeviafan opened this issue Dec 8, 2022 · 13 comments · Fixed by #643
Closed

Bookmarklet handling form in bukuserver ignores entered values #639

LeXofLeviafan opened this issue Dec 8, 2022 · 13 comments · Fixed by #643
Labels

Comments

@LeXofLeviafan
Copy link
Collaborator

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
Copy link
Collaborator Author

LeXofLeviafan commented Dec 8, 2022

…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
Copy link
Collaborator

to summarize

  1. submitted description and title is not added when created record
  2. 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

  1. disable all auto fill on all record created from bukuserver
  2. 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:

  1. go to https://github.com/jarun/buku, make sure it is not added to buku yet
  2. select this line
For those who prefer the GUI, bukuserver exposes a browsable front-end on a local web host server.
  1. go to bukuserver last bookmark page
  2. 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
Copy link
Collaborator Author

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:

  1. open a page which hasn't been added to buku yet
  2. click the bookmarklet
  3. edit every field in the form (e.g. add #foo to Url, replace Title with bar, replace Description with baz)
  4. 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
Copy link
Collaborator Author

i assume there is

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

@rachmadaniHaryono
Copy link
Collaborator

rachmadaniHaryono commented Dec 8, 2022

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

  1. open https://github.com/jarun/buku which hasn't been added to buku yet
    1.2 select text Personal
  2. click the bookmarklet
  3. replace Description with baz
  4. 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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

LeXofLeviafan commented Dec 8, 2022

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
Copy link
Collaborator

rachmadaniHaryono commented Dec 8, 2022

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

  1. go to edit page on first tab
  2. open new tab and delete that bookmark
  3. 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 ...

buku/buku

Line 610 in bddc938

DB index of new bookmark on success, -1 on failure.

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

that one? or you mean something else

@LeXofLeviafan
Copy link
Collaborator Author

LeXofLeviafan commented Dec 8, 2022

  1. open new tab and delete that bookmark
  2. on first tab click details button

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

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
Copy link
Collaborator

rachmadaniHaryono commented Dec 8, 2022

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 added a commit to LeXofLeviafan/buku that referenced this issue Dec 8, 2022
@LeXofLeviafan
Copy link
Collaborator Author

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 bf0f7af 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)

👍

LeXofLeviafan added a commit to LeXofLeviafan/buku that referenced this issue Dec 11, 2022
@LeXofLeviafan
Copy link
Collaborator Author

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
Copy link
Collaborator Author

something like this on example section

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants