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

web: Import & export, permissions from CLI or headers #821

Merged
merged 25 commits into from
Jun 29, 2018

Conversation

zarybnicky
Copy link
Contributor

More or less a complete overhaul of hledger-web - the diff is quite scary...

I've attempted to split it into well-defined commits but at the end, I was moving around large chunks of code, so the resulting diff is more-or-less "delete everything & write it anew"...

I've added two new CLI flags: --capabilities, which statically controls the capabilities of the application, and --capabilities-header, which overrides the previous flag and enables reading the (comma-separated) capabilities from the header given as a parameter.

Also, I've resurrected, finished, and refactored the import and edit forms (of which only one of them had been working at all, not sure which one), added a separate 'Manage journal files' page with a list of all the files the current journal is composed of (and edit/upload/download buttons for each of them).

In addition, having all of the forms as separate endpoints for both GET and POST plays well with yesod-form's error messages - after submitting the add form, I redirect the user to /add (with just the add form), where the error message is displayed next to the relevant field.

This PR also addresses a few other issues:

I haven't actually tested it on Sandstorm, only locally - I haven't really looked into what's needed for that. I've defined the permissions in the *.capnproto file, edited the launcher.sh file, so the only thing remaining is to test it. I've however ran out of time in the short term, so I've finished all the changes to code and opened this PR...

@zarybnicky
Copy link
Contributor Author

I forgot to mention - there are no immediate eye catching differences. Only when you start it with hledger-web --capabilities=view,add,manage, then there is an extra wrench icon next to the search bar, which opens the 'Manage' UI

@zarybnicky
Copy link
Contributor Author

Ok, building with make buildtests-all says that I need to either update yesod or make a CPP'd type alias for WidgetT vs WidgetFor...

@simonmichael simonmichael added A-WISH Some kind of improvement request, hare-brained proposal, or plea. web The hledger-web tool. help wanted needs:testing To unblock: needs more developer testing or general usage labels Jun 29, 2018
@simonmichael simonmichael added the needs:review To unblock: needs more code/docs/design review by someone label Jun 29, 2018
@simonmichael
Copy link
Owner

Looks good! Thank you!

Providing an invalid capability name to --capability causes it to serve "Internal Server Error", it should give a command-line error.

When started without the view capability, perhaps it could also hide the journal's file name.

--capability sounds like the right technical term, but I don't love it for the user experience. Is there something more friendly and familiar - --access, --permission[s], ... ?

I will go ahead and merge, as a release is due tomorrow.

I will just say the addform (and perhaps the others in the past) were in the default template for a reason: to make them always available so that you could hit a to start data entry no matter where you navigate in the app. Lack of this may become more noticeable if we add more screens. But perhaps the old way was ugly and unsustainable.

@simonmichael simonmichael merged commit 282cfbd into simonmichael:master Jun 29, 2018
@simonmichael simonmichael added platform:sandstorm The Sandstorm.io web app hosting platform. needs:docs To unblock: needs corresponding documentation or doc updates needs:changes To unblock: needs some changes made, in line with recommendations and removed needs:review To unblock: needs more code/docs/design review by someone needs:testing To unblock: needs more developer testing or general usage help wanted labels Jun 29, 2018
@simonmichael
Copy link
Owner

PS I see this does even more than I noticed, I'll have to read the linked issues. And, I forgot about docs. We should document the changes in the hledger-web manual (and the web help dialog if you think it appropriate).

@simonmichael
Copy link
Owner

I think I also misread the CI status. stack test hledger-web no longer builds:

[1 of 4] Compiling TestImport       ( tests/TestImport.hs, .stack-work/dist/x86_64-osx/Cabal-2.2.0.1/build/test/test-tmp/TestImport.o ) [Foundation changed]

/Users/simon/src/PLAINTEXTACCOUNTING/hledger/hledger-web/tests/TestImport.hs:10:1: error:
    Could not find module ‘Foundation’
    Use -v to see a list of the files searched for.
   |
10 | import Foundation
   | ^^^^^^^^^^^^^^^^^

After that we should also check make buildtest-all passes. I can run that one if necessary.

@simonmichael simonmichael added the needs:testing To unblock: needs more developer testing or general usage label Jun 29, 2018
@zarybnicky
Copy link
Contributor Author

I'll respond more in the evening but for now - make buildtest-all works with the last two commits on this branch, where I've fixed these build issues - that's what the green checkmark should mean, right?

@zarybnicky
Copy link
Contributor Author

Nevermind, I don't remember changing the testsuite - I only recall considering removing it, as right now, it is AFAIK useless.

@simonmichael
Copy link
Owner

@zarybnicky: I disabled hledger-web's test suite for now.

How would you feel about changing the zero toggle key to Z, for consistency with hledger-ui and to free up the valuable e/E keys for edit operations ? See #641 (comment) for more context.

@zarybnicky
Copy link
Contributor Author

zarybnicky commented Jul 2, 2018

@simonmichael I was about to get into this again, at least for today :)

How would you feel about changing the zero toggle key to Z, for consistency with hledger-ui and to free up the valuable e/E keys for edit operations ? See #641 (comment) for more context.

Yes, I saw that comment earlier today. I have no strong feelings either way, I'll include z in the next hledger-web PR - together with #834 and perhaps more.

I disabled hledger-web's test suite for now.

Currently, the tests do nothing even if they compile (well, they test that the app starts at all...). I'm not sure how valuable yesod-test is here - though I have next to no experience with Selenium or other integration test frameworks for webapps (I only ever used unittests for both frontend JS and backend). Though considering that we've added permissions to the mix, it might be worth filling the testsuite out.

Providing an invalid capability name to --capability causes it to serve "Internal Server Error", it should give a command-line error.

Will look into it.

When started without the view capability, perhaps it could also hide the journal's file name.

An oversight on my part, I knew I forgot something...

--capability sounds like the right technical term, but I don't love it for the user experience. Is there something more friendly and familiar - --access, --permission[s], ... ?

See #834, which I've opened for further discussion.

I will just say the addform (and perhaps the others in the past) were in the default template for a reason: to make them always available so that you could hit a to start data entry no matter where you navigate in the app. Lack of this may become more noticeable if we add more screens. But perhaps the old way was ugly and unsustainable.

I'm aware of that. I've tried leaving it in, but Bloodhound (the JS auto-completion library) refused to work when there were two addForms on the page (on /add, after POSTing a badly filled out add form). I'm sure there's a way to fix that, but supporting two add forms on a single page requires a lot of JS changes - this was simply the easier solution.

What I could add is a Manage journal modal window - keeping the /manage page for JS-disabled browsers, but adding an onClick handler and a keybinding (m?) for opening it.

Edit: Oh, and docs, that's another thing.

@zarybnicky zarybnicky deleted the web_permissions branch July 2, 2018 09:53
@ocdtrekkie
Copy link
Contributor

Oh, wow. I never saw this PR. I need to look at updating the Sandstorm package!

@zarybnicky
Copy link
Contributor Author

@ocdtrekkie If you do, can you look if the protobuf settings for permissions are correct? I meant to look into testing Sandstorm before, but I never got around to it...

@ocdtrekkie
Copy link
Contributor

Yeah, I've got a big to-do backlog at the moment, but I'd absolutely test those out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. needs:changes To unblock: needs some changes made, in line with recommendations needs:docs To unblock: needs corresponding documentation or doc updates needs:testing To unblock: needs more developer testing or general usage platform:sandstorm The Sandstorm.io web app hosting platform. web The hledger-web tool.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants