search

jonaswinkler / paperless-ng

A supercharged version of paperless: scan, index and archive all your physical documents
https://paperless-ng.readthedocs.io/en/latest/
GNU General Public License v3.0
5.37k stars 356 forks source link

Consumer status on the dashboard #415

Closed jonaswinkler closed 3 years ago

jonaswinkler commented 3 years ago

Add ability to see the status of the document consumer per document on the dashboard

This will not work with Apache mod_wsgi, since this change requires me to use web sockets and make paperless an asgi application. mod_wsgi will continue to work, you'll simply not get the status notifications in the front end.

shamoon commented 3 years ago

Again, offering my help if you'd like.

jonaswinkler commented 3 years ago

https://user-images.githubusercontent.com/17569239/105616231-c9ccee00-5dd5-11eb-8382-19406c642e4a.mp4

Something like this. This will naturally work with files dropped in the consumption folder as well as with documents from mails.

shamoon commented 3 years ago

Looks great! Any thought to just making the toast like that?

jonaswinkler commented 3 years ago

Any thought to just making the toast like that?

Like what?

shamoon commented 3 years ago

As in put that widget with the document status progress bar into a notification 'toast' itself, when it's done it shows the link

jonaswinkler commented 3 years ago

I am not entirely sure yet. What I still want do to is

Toasts are supposed to go away after some time; and these would certainly stay until done.

Also: what happens if someone drops 20 documents at once; that gets pretty messy really fast.

shamoon commented 3 years ago

Yea just spitballing. Agree would be a longer-lasting toast (but maybe could still expire). Re: multiple docs, good point. Not sure if could still use one notification. And could visually squash things, progress bar just a few pixels tall, smaller text etc then stacking a bunch not such a big deal (but still a limit)

But yea just thinking out loud

jonaswinkler commented 3 years ago

@shamoon Would you like to work on the UI for this? I've made the necessary changes to the back end and added a service in the front end that both handle the logic.

Development for this is on branch feature-websockets-status, which is a fast forward from dev right now. It's a pretty big change (new web server that supports web sockets) and many new dependencies. I'd like to see if this works properly first.

Things I thought about:

shamoon commented 3 years ago

Yea sounds fun! Will dig into it

shamoon commented 3 years ago

Hey! This is awesome, great job (as usual). I encountered one small bug which is that the first call of getProgress in consumer-status.service.ts:30 seems to report a currentPhaseProgress that is out of whack, for example if I log the values for this.phase, this.currentPhaseProgress, this.currentPhaseMaxProgress I get 2 102156 100. After the first call its fine. This only matters because it makes the progress bar start at 100 and drop down. I have a hacky-fix in-place for testing where just make it 0 if its > 100.

Anyway, would love your feedback on this, its definitely not finished but I'm liking it. A few notes / responses:

Look forward to your thoughts! (see vid):

https://user-images.githubusercontent.com/4887959/105841702-e4e05f00-5f89-11eb-91fc-a624f1a34574.mov

jonaswinkler commented 3 years ago

Hey! This is awesome, great job (as usual). I encountered one small bug which is that the first call of getProgress in consumer-status.service.ts:30 seems to report a currentPhaseProgress that is out of whack, for example if I log the values for this.phase, this.currentPhaseProgress, this.currentPhaseMaxProgress I get 2 102156 100. After the first call its fine. This only matters because it makes the progress bar start at 100 and drop down. I have a hacky-fix in-place for testing where just make it 0 if its > 100.

Oh, that's because updateProgress() further down in that file does not update currentPhaseProgress when the parameter currentProgress is 0, since 0 is not true. Care to fix that?

Anyway, would love your feedback on this, its definitely not finished but I'm liking it. A few notes / responses:

  • Im not sold on dismissing old alerts / setting a limit. I implemented the "hide all" button and the widget is a block element so it just grows taller & window can scroll, not so crazy I think. And I feel like if we hid them automatically people would get confused like "what happened to the other 10 docs I dropped in?". In that case we'd need something at the bottom like "10 more documents..." Not sure, it is still very tall with e.g. 20 docs
  • My issue with sorting is the alerts will start jumping around which feels kinda jarring. I think the new UI (see video) makes it easily apparent which items are done / error / in progress and so maybe less need for sorting? Ya know, IMHO.

True. Also, you'll probably anticipate clicking the open button when consumption is almost done, and it's pretty bad to move that button elsewhere.

  • Put this in the only place that made sense / wasnt obtrusive to me, widget top. It fades in only when needed, ofc

Looks good to me.

  • Single line is tempting but file titles are often very long so I think wrapping is inevitable. I did go for X dismiss button and small open link.

Look forward to your thoughts! (see vid):

consumer_alerts.mov

Progress bar in the background is a very good idea.

In general, I'm just concerned about users that will initially drop a couple 100 documents in there, or the consumption directory, and they'll get flooded with notifications and the widget won't be very helpful at all. I'd like to make sure that especially new users get a good initial impression.

I'm considering to add configuration options to enable / disable the toasts.

jonaswinkler commented 3 years ago

Also, try adding a view that shows recently added documents on the dashboard ;)

shamoon commented 3 years ago

Oh, that's because updateProgress() further down in that file does not update currentPhaseProgress when the parameter currentProgress is 0, since 0 is not true. Care to fix that?

Ah I see, done =)

Looks good to me.

👍🏼

Progress bar in the background is a very good idea.

👍🏼

In general, I'm just concerned about users that will initially drop a couple 100 documents in there, or the consumption directory, and they'll get flooded with notifications and the widget won't be very helpful at all. I'd like to make sure that especially new users get a good initial impression.

Good point. Ok I set an arbitrary maximum of 5 (10?) and added text and a button to show them. It works pretty well I think! See video (video is max 3 items just for illustration):

https://user-images.githubusercontent.com/4887959/105915671-d8d5bb00-5fe4-11eb-97c7-7ca7380d0ee9.mov

I'm considering to add configuration options to enable / disable the toasts.

You mean a global config, like disable all toasts? I was wondering if we should not show consumer toasts if user is on the dashboard since they'll have the alerts but especially since were going to have a maximum amount I dont think we should i.e. a document finishes that's alert is already gone.

Also, try adding a view that shows recently added documents on the dashboard ;)

Awesome, love how it live updates!

jonaswinkler commented 3 years ago

You mean a global config, like disable all toasts?

Not all toasts, but two options in the settings for

I was wondering if we should not show consumer toasts if user is on the dashboard since they'll have the alerts

Not sure. It's good to have consistency, but I see your point.

but especially since were going to have a maximum amount I dont think we should i.e. a document finishes that's alert is already gone.

That's pretty hard to understand.

jonaswinkler commented 3 years ago

Care to make a PR with the current state? I'd like to have a look.

shamoon commented 3 years ago

Yea! See #452

That's pretty hard to understand.

Yea sorry I see typos made that confusing. I just meant if an alert in the widget is hidden or dismissed the toast would still be useful. But giving config options would work too ofc.

jonaswinkler commented 3 years ago

Dismiss all should only remove completed / failed items.

shamoon commented 3 years ago

Ah yes I forgot to do that. I can make that and any other changes I’d you like, LMK

jonaswinkler commented 3 years ago

~I'm currently considering this to bring some more structure into the widget:~

~Do you think this would be a good solution?~

Scratch that, I think this is pretty good.

shamoon commented 3 years ago

Kind of a toss-up. It obviously helps in the lots-of-documents situation but I think it sounds slightly worse when dropping only a few.

To my mind, if you drop 10/20/100 documents in you’d care less about each individual status or the “open” button for each document, whereas the person dropping 1 or 2 is probably more likely to want those.

Of course toasts will have an open button and it’s not like it’s a “big deal” to just expand the list if you want to see individual progress. But toasts disappear. Also, even if we use one overall progress bar the expand would still be huge in the case of eg. 100 docs.

Also I think partly I just find the individual alerts very enjoyable to watch which makes me not to want to hide them 😆 but yea, I get the argument for the one overall progress bar

shamoon commented 3 years ago

Scratch that, I think this is pretty good.

Ha, agreed :)

jonaswinkler commented 3 years ago

@shamoon Mind giving the new docker image (tag name feature-websockets-status) a quick test run?

shamoon commented 3 years ago

@shamoon Mind giving the new docker image (tag name feature-websockets-status) a quick test run?

Sure thing, should have a chance a little later on. Do you want me to try a fresh install or more interested in updating an existing install? (Or both 😀)

jonaswinkler commented 3 years ago

Well, both would be good, but no guarantees on dev versions.

shamoon commented 3 years ago

Fresh install: Awesome! Existing install: Awesome!

You did end up hiding toasts on dashboard—I totally agree. It feels great, Im watching one area for progress but if I leave I still get to know what happened.

Bravo, as usual! 👏🏼

jonaswinkler commented 3 years ago

@C0nsultant I might need a little input here.

 TASK [ansible : enable paperlessng services] ***********************************
  ok: [debian_buster] => (item=paperless-consumer)
  ok: [ubuntu_focal] => (item=paperless-consumer)
  ok: [debian_buster] => (item=paperless-scheduler)
  ok: [ubuntu_focal] => (item=paperless-scheduler)
  changed: [debian_buster] => (item=paperless-webserver)
  changed: [ubuntu_focal] => (item=paperless-webserver)

  PLAY RECAP *********************************************************************
  debian_buster              : ok=27   changed=1    unreachable=0    failed=0    skipped=13   rescued=0    ignored=0
  ubuntu_focal               : ok=27   changed=1    unreachable=0    failed=0    skipped=13   rescued=0    ignored=0

  CRITICAL Idempotence test failed because of the following tasks:
  *  => ansible : enable paperlessng services
  *  => ansible : enable paperlessng services
WARNING  An error occurred during the test sequence action: 'idempotence'. Cleaning up.
C0nsultant commented 3 years ago

I've made the change in the tasks/main.yml

Did I do anything wrong?

Your changes are correct. Thanks for tackling the ansible role, btw. The problem is based on the fact that I still don't have #411 (finished and) merged. You see, the current molecule test on master and dev pulls NG sources from a published release archive. Your changes to the role (in particular the introduction of uvicorn.workers.UvicornWorker into paperless-webserver.service) are incompatible with that old release. It simply breaks the webserver because dependencies introduced in this PR are not tracked:

root@ansible:/opt/tmp/paperless-ng# systemctl status paperless-webserver
* paperless-webserver.service - Paperless webserver
   Loaded: loaded (/etc/systemd/system/paperless-webserver.service; enabled; vendor preset: enabled)
   Active: failed (Result: exit-code) since Sat 2021-01-30 08:10:15 UTC; 9s ago
  Process: 15213 ExecStart=/opt/paperless-ng/.venv/bin/gunicorn paperless.asgi:application -w 2 -k uvicorn.workers.UvicornWorker -b 0.0.0.0:8000 (code=exited, status=1/FAILURE)
 Main PID: 15213 (code=exited, status=1/FAILURE)

Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 983, in _find_and_load
Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 983, in _find_and_load
Jan 30 08:10:15 ansible gunicorn[15213]:   File "<frozen importlib._bootstrap>", line 965, in _find_and_load_unlocked
Jan 30 08:10:15 ansible gunicorn[15213]: ModuleNotFoundError: No module named 'uvicorn'
Jan 30 08:10:15 ansible gunicorn[15213]: ]
Jan 30 08:10:15 ansible systemd[1]: paperless-webserver.service: Main process exited, code=exited, status=1/FAILURE
Jan 30 08:10:15 ansible systemd[1]: paperless-webserver.service: Failed with result 'exit-code'.`

I will get to work on that PR right now and get it ready ASAP.

I also just figured that it would probably be best to use the gunicorn.conf.py file for configuration here as well, instead of specifying that on the command line. All options in one place.

That would presumably be the same way it is done in https://github.com/jonaswinkler/paperless-ng/blob/master/docker/supervisord.conf#L11? I can get to work on that if you want. Just not sure where to put the configuration yet.

jonaswinkler commented 3 years ago

I see! Dependencies are installed from the latest release, and that does not have the added requirements. Would have been good to see that error somewhere in the CI logs, or maybe I'm just blind.

That would presumably be the same way it is done in https://github.com/jonaswinkler/paperless-ng/blob/master/docker/supervisord.conf#L11? I can get to work on that if you want. Just not sure where to put the configuration yet.

Yes. Not all that important. I think I'd like to leave that as an excercise for myself, actually.

C0nsultant commented 3 years ago

Would have been good to see that error somewhere in the CI logs, or maybe I'm just blind.

The unfortunate thing about starting a systemd service with ansible is that there is no verification about the service being active. When the start triggers successfully and then crashes, ansible reports started as OK. It's a good idea to add a verification step to the role to check whether the service is healthy. Catching it in the idempotence test is too weird of a side-effect.

I think I'd like to leave that as an excercise for myself, actually.

Sure. One module that will certainly help you is lineinfile.

jonaswinkler commented 3 years ago

@C0nsultant

The unfortunate thing about starting a systemd service with ansible is that there is no verification about the service being active. When the start triggers successfully and then crashes, ansible reports started as OK. It's a good idea to add a verification step to the role to check whether the service is healthy. Catching it in the idempotence test is too weird of a side-effect.

Alright.

Please see https://github.com/jonaswinkler/paperless-ng/runs/1797890630?check_suite_focus=true#step:6:678. That's because the specified references aren't fetched. For example refs/heads/dev is not available, but refs/remotes/origin/dev is. Two options, probably:

Also, the current refspec is different from what's in the examples (https://docs.ansible.com/ansible/latest/collections/ansible/builtin/git_module.html#examples). Not sure if that's deliberate or by accident.

Furthermore, no effect on the branch feature-websockets-status, still the same as before:

By the way, these test cases are pretty solid. I don't have any automated deployment tests over here, and most of the major issues so far were related to packaging and deployment, so these tests really help out a lot. Therefore: Thank you for all the effort!

C0nsultant commented 3 years ago

That's because the specified references aren't fetched. For example refs/heads/dev is not available, but refs/remotes/origin/dev is.

My bad. I was only thinking about the tests in the context of a PR. As you correctly identified, this happens partly because the refspec: argument in the call to pull the repository only fetches ref/pull/s (in addition to default refs like remote branches).

adjust the refspec in install-source.yml, line 32, to include these

Ideally, I would like to pull all ref/*s from the remote to the local checkout, but I haven't figured out how to do so (and if it is even possible). If you have any input, please share. [I was completely unfamiliar with refspecs prior to this, please pardon any ignorance regarding their use.]

the current refspec is different from what's in the examples ... Not sure if that's deliberate or by accident.

Not an accident, by design. As for the reason why it differs from the one in the ansible examples: For PRs, GITHUB_REF is of the form refs/pull/:prNumber/merge. Pulling a PR branch from a foreign remote is possible using that ref (at least when using GitHub). In order not to have to re-write refs, I designed the refspec to simply mirror all refs/pull/* specs from the remote (jonaswinkler/paperless-ng) to the local checkout under the same name. The example in the ansible documentation renames the refs, requiring an additional step when translating GITHUB_REF into something that exists locally.

change how paperlessng_version is derived from GITHUB_REF to reflect the above

GitHub actions at least always spit out a GITHUB_SHA for any push or PR. paperlessng_version also can be specified as a commit hash, so maybe it is best to use that. No more worries about ref names (the refs have to be available, though - using a commit hash from a PR will result in failure if the PR ref is not available locally). #474

C0nsultant commented 3 years ago

Tests fail due to the new options and the specified gunicorn worker not being available.

Are you talking about this? I changed the molecule steps from the default molecule test (with its default scenario order) to something that is faster in the NG case. You probably are expecting a different error because of that.

This leads me to believe that the scripts still use requirements ... from a release

That is correct. The role tests both installation of the latest release as well as the current GITHUB_REF source. The prior fails in this case for the same reason outlined in an earlier comment: -k uvicorn.workers.UvicornWorker (which gets applied to both installations) is incompatible with the requirements.txt of 1.0.0. I don't know enough about gunicorn, but can the worker_class (-k) be incorporated into your gunicorn.conf.py approach? Either way, this probably requires a conditional to include or exclude the worker class for paperlessng_version > 1.0.0 or paperlessng_version == 1.0.0 (#473). The absolute cleanest way to do it might be git merge-base --is-ancestor to determine whether the commit (hash) of paperlessng_version is before or after a breaking change commit (hash).

Thank you for all the effort!

Very happy to help in getting this project to the NG ;)

jonaswinkler commented 3 years ago

I don't know enough about gunicorn, but can the worker_class (-k) be incorporated into your gunicorn.conf.py approach?

Absolutely. Thanks for the explanation on the order of execution, that helps alot. I'll get to it. Once this new configuration is on all branches, this won't be an issue anymore anyway as far as I can tell.