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

bugfix bukuserver #546

Merged
merged 33 commits into from
Jan 22, 2022
Merged

bugfix bukuserver #546

merged 33 commits into from
Jan 22, 2022

Conversation

rachmadaniHaryono
Copy link
Collaborator

@rachmadaniHaryono rachmadaniHaryono commented Jan 6, 2022

resolves #543
resolves #545
resolves #547
resolves #548
resolves #553
resolves #554
resolves #559

  • pre-commit
    • update
    • use local pylint
  • fix import server error on bukuserver.__main__
  • isort
    • bukuserver.forms
    • bukuserver.server
    • bukuserver.views
    • tests.test_buku
  • don't validate url, let buku do it
  • bukuserver
    • use its own search_tag func
    • use fork of flask-reverse-proxy-fix
  • pylint fix BookmarkletView too few public method
  • tests.test_buku
    • merge test_parse_tags
    • new test_parse_tags_no_args
  • simplify test_bookmark_model_view
  • flake8 add e203 for black compatilibityh
  • bukuserver.views
    • handle unbound choosen_idx CustomAdminIndexView.search
    • BookmarkModelView._apply_filters
      • remove unused flt_name var
      • handle no self._filters
    • BookmarkModelView
      • _apply_filters
        • black formatted
        • dont manually check scheme validness
      • use buku.parse_tags on create_model and update_model
      • check arg when creating new bookmark record
    • use single message on exception and error flash
    • black formatted
    • dont use favicon when there is no netloc

@rachmadaniHaryono rachmadaniHaryono marked this pull request as draft January 7, 2022 02:48
@rachmadaniHaryono rachmadaniHaryono mentioned this pull request Jan 9, 2022
97 tasks
@jarun
Copy link
Owner

jarun commented Jan 9, 2022

@rachmadaniHaryono anything left to be done here?

@rachmadaniHaryono
Copy link
Collaborator Author

#548, #547 could be here

i would decline #549 because it is feature request

i haven't see #543 and #553 yet

but no work for today, and maybe start from tomorrow

- use f string
- don't use string as result variablee
- black formatted
@rachmadaniHaryono rachmadaniHaryono changed the title ftp link bugfix bukuserver Jan 10, 2022
@jarun
Copy link
Owner

jarun commented Jan 12, 2022

OK. Undraft when ready and please leave a note for me.

@rachmadaniHaryono
Copy link
Collaborator Author

while you are here please also read #484 (comment)

i need your opinion on that

@rachmadaniHaryono rachmadaniHaryono marked this pull request as ready for review January 14, 2022 06:38
@rachmadaniHaryono
Copy link
Collaborator Author

rachmadaniHaryono commented Jan 14, 2022

@GreenLunar can you confirm this pr work for your issue

if yes then @jarun can merge it if jarun agree with this pr

@GreenLunar
Copy link

It has been a while since I've utilized git.
Please tell me how to integrate this PR with the most recent master source archive.
Then, I'll test and report back.

@rachmadaniHaryono
Copy link
Collaborator Author

rachmadaniHaryono commented Jan 14, 2022

if you use pipx/pip you can download it from the zip

pipx install https://github.com/rachmadaniHaryono/Buku/archive/refs/heads/bugfix/ftp-link.zip

e: from source

> git remote add rachmadaniHaryono https://github.com/rachmadaniHaryono/Buku.git
> git checkout rachmadaniHaryono/bugfix/ftp-link

@GreenLunar
Copy link

Hi, I'll test and send a feedback. I'll be able to begin testing within 26 hours or so. I beg your pardon for my delay. I'll get to it ASAP.

@rachmadaniHaryono
Copy link
Collaborator Author

dont' worry and take your time

@jarun
Copy link
Owner

jarun commented Jan 17, 2022

@rachmadaniHaryono

while you are here please also read #484 (comment)

i need your opinion on that

I am fine. Just ensure if someone wants to install only the cmdline part, she doesn't have to install everything.

@rachmadaniHaryono
Copy link
Collaborator Author

rachmadaniHaryono commented Jan 18, 2022

i added my fork of flask-reverse-proxy-fix so user don't have to manually install my fork

i also update note here #449

@jarun
Copy link
Owner

jarun commented Jan 19, 2022

@rachmadaniHaryono OK to merge?

@rachmadaniHaryono
Copy link
Collaborator Author

i actually wait for @GreenLunar #546 (comment)

but it is already 5 day and ci is passed, so maybe merge it and close related issue

if there is bug please open new pr or reopen related issue


while you are here, i also make mistake by merging commit accidentally c09ecfb

so revert that commit before merging this pr

related #484 (comment)

@GreenLunar
Copy link

Apologies for the procrastination. I've got work tasks on my shoulders.
I'll dedicate the time for it, today. Within 24 hours from now, you'll get my report.

Again, sorry about it.

@GreenLunar
Copy link

Seems good

@GreenLunar
Copy link

GreenLunar commented Jan 20, 2022

Hi, I've tested all from a remote machine and all worked well.
I want to make more tests, and so I'm attempting to try this at home too.

Below are obstacles which I'm trying to resolve.

> git init
> git remote add rachmadaniHaryono https://github.com/rachmadaniHaryono/Buku.git
> git checkout rachmadaniHaryono/bugfix/ftp-link
$ git checkout rachmadaniHaryono/bugfix/ftp-link
error: pathspec 'rachmadaniHaryono/bugfix/ftp-link' did not match any file(s) known to git

I went on trying pipx for the first time:

$ bukuserver run --host 127.0.0.1 --port 5001
Traceback (most recent call last):
  File "/home/HOME/.local/bin/bukuserver", line 5, in <module>
    from bukuserver.server import cli
  File "/home/HOME/.local/pipx/venvs/buku/lib/python3.10/site-packages/bukuserver/server.py", line 12, in <module>
    from flask.cli import FlaskGroup
ModuleNotFoundError: No module named 'flask'

Trying to install dependencies leads me to this message: "No apps associated with package PACKAGE or its dependencies. If you are attempting to install a library, pipx should not be
used. Consider using pip or a similar tool instead."

e: Just in case:

$ pip install buku
Defaulting to user installation because normal site-packages is not writeable
Requirement already satisfied: buku in /usr/lib/python3.10/site-packages (4.6)
Requirement already satisfied: beautifulsoup4>=4.4.1 in /usr/lib/python3.10/site-packages (from buku) (4.10.0)
Requirement already satisfied: certifi in /usr/lib/python3.10/site-packages (from buku) (2021.10.8)
Requirement already satisfied: cryptography>=1.2.3 in /usr/lib/python3.10/site-packages (from buku) (36.0.1)
Requirement already satisfied: urllib3>=1.23 in /usr/lib/python3.10/site-packages (from buku) (1.26.7)
Requirement already satisfied: html5lib>=1.0.1 in /usr/lib/python3.10/site-packages (from buku) (1.1)
Requirement already satisfied: soupsieve>1.2 in /usr/lib/python3.10/site-packages (from beautifulsoup4>=4.4.1->buku) (2.2.1)
Requirement already satisfied: cffi>=1.12 in /usr/lib/python3.10/site-packages (from cryptography>=1.2.3->buku) (1.15.0)
Requirement already satisfied: pycparser in /usr/lib/python3.10/site-packages (from cffi>=1.12->cryptography>=1.2.3->buku) (2.21)
Requirement already satisfied: six>=1.9 in /usr/lib/python3.10/site-packages (from html5lib>=1.0.1->buku) (1.16.0)
Requirement already satisfied: webencodings in /usr/lib/python3.10/site-packages (from html5lib>=1.0.1->buku) (0.5.1)
$ pip install flask
Defaulting to user installation because normal site-packages is not writeable
Requirement already satisfied: flask in ./.local/lib/python3.10/site-packages (1.1.4)
Requirement already satisfied: Werkzeug<2.0,>=0.15 in ./.local/lib/python3.10/site-packages (from flask) (1.0.1)
Requirement already satisfied: click<8.0,>=5.1 in ./.local/lib/python3.10/site-packages (from flask) (7.1.2)
Requirement already satisfied: itsdangerous<2.0,>=0.24 in ./.local/lib/python3.10/site-packages (from flask) (1.1.0)
Requirement already satisfied: Jinja2<3.0,>=2.10.1 in ./.local/lib/python3.10/site-packages (from flask) (2.11.3)
Requirement already satisfied: MarkupSafe>=0.23 in /usr/lib/python3.10/site-packages (from Jinja2<3.0,>=2.10.1->flask) (2.0.1)

@GreenLunar
Copy link

@rachmadaniHaryono I've commented on the issues one by one. Please let me know if I've missed anything.

@rachmadaniHaryono
Copy link
Collaborator Author

rachmadaniHaryono commented Jan 21, 2022

based on this comment 1 seem like you have difficulties getting my branch.

but after that you can get it working by this comment 2.

so i assume this is working,

if not see below


> mkdir /tmp/tmp2
> cd /tmp/tmp2
> git clone https://github.com/rachmadaniHaryono/Buku.git
> cd Buku
> git checkout bugfix/ftp-link
> # add --force flag if necessary
> pipx install ".[server]"
> bukuserver run
> # after finished trying the branch
> pipx uninstall buku

this 3 should be fixed on 4

unfortunately for 5, i may have to put it on new pr

so basically if @GreenLunar think 3 is fixed at this point, this pr can be merged

for @jarun, can you check if sql command is correct on search_tag function in bukuserver.server module

e: note remind me to put above shell example on documentation

Footnotes

  1. https://github.com/jarun/buku/pull/546#issuecomment-1017281578

  2. https://github.com/jarun/buku/pull/546#issuecomment-1017371333

  3. https://github.com/jarun/buku/issues/554 2

  4. https://github.com/jarun/buku/pull/546/commits/715d0c9b2d3bb177f320d1bbe03705e2065c127c

  5. https://github.com/jarun/buku/issues/559

@jarun
Copy link
Owner

jarun commented Jan 22, 2022

while you are here, i also make mistake by merging commit accidentally c09ecfb

done!

@jarun jarun merged commit c7243ad into jarun:master Jan 22, 2022
@jarun
Copy link
Owner

jarun commented Jan 22, 2022

Thank you so much guys!

@GreenLunar
Copy link

GreenLunar commented Jan 22, 2022

based on this comment 1 seem like you have difficulties getting my branch.

but after that you can get it working by this comment 2.

so i assume this is working,

Yes, it's working and tested.

if not see below

> mkdir /tmp/tmp2
> cd /tmp/tmp2
> git clone https://github.com/rachmadaniHaryono/Buku.git
> cd Buku
> git checkout bugfix/ftp-link
> # add --force flag if necessary
> pipx install ".[server]"
> bukuserver run
> # after finished trying the branch
> pipx uninstall buku

As I recall, I've managed to install using pip.
Thanks. I'll follow the git steps for the next pull request.
As much as I dislike QA, I'll make more tests on the next pull request, again ;)

@rachmadaniHaryono rachmadaniHaryono deleted the bugfix/ftp-link branch January 22, 2022 19:44
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.