arXiv / arxiv-submission-ui

User interface of NG submit system.
MIT License
2 stars 6 forks source link

ARXIVNG-1190 ARXIVNG-1191 Implemented stage control, added CSRF protection #33

Closed erickpeirson closed 6 years ago

erickpeirson commented 6 years ago

The main objective is to be able to enforce the decision that users must complete required stages in order. To do that, I introduced a new domain class called SubmissionStage, which represents the state of the submission with regard to the submission UI steps. To make everything nice and tidy, I refactored the flow control mechanism to use the SubmissionStage class. I also wired up the progress bar.

If a user tries to jump ahead past a required step, they are redirected back to the appropriate stage (the current stage == the stage after the last completed stage, i.e. what the user should do next), and a message is flashed.

image

Along the way, I noticed that we're not doing anything to protect against CSRF attacks. So I rebased all of the forms used in controllers on arxiv.base.forms.csrf.CSRFForm. This kind of exploded the number of files changed, since this meant also updating all of the controller tests with appropriate mocks.

Finally, I started on an application-level test suite (tests/test_workflow.py), that exercises the whole app. We should keep adding to that as the interface is finalized.

This branch has changes from #31 and #32, so those will need to be merged first. Note that #32 also depends on https://github.com/cul-it/arxiv-filemanager/pull/20.

@eawoods Once those other PRs are merged, can you verify expected behavior? @DavidLFielding @JaimieMurdock Please feel free to review if you have time, but don't let it distract you from other critical work.

Will plan to clear these by COB today (18 September).

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 302


Changes Missing Coverage Covered Lines Changed/Added Lines %
submit/controllers/create.py 6 7 85.71%
submit/util.py 14 15 93.33%
submit/domain.py 102 117 87.18%
submit/routes/ui.py 45 60 75.0%
submit/routes/util.py 43 63 68.25%
<!-- Total: 251 303 82.84% -->
Totals Coverage Status
Change from base Build 296: 10.6%
Covered Lines: 1115
Relevant Lines: 1444

💛 - Coveralls
eawoods commented 6 years ago

Yes I will test when appropriate - I just approved #31 and #32. Does filemanager #20 also need to be merged before testing, or can I check out that branch to test this PR?

erickpeirson commented 6 years ago

Bounced base to update diffs

erickpeirson commented 6 years ago

Yeah, those seem right. Struggled with what those scenarios should look like. Will take another pass when I'm fresh in the AM

eawoods commented 6 years ago

Awesome. I should be available tomorrow morning (early) and around an appointment window, if you want to collaborate (screenshots, css, whatnot) or if I can help at all.

erickpeirson commented 6 years ago

@eawoods Ok, I think that I've achieved those two requests. I also added some labels to the SubmissionStage class, for use in the flash message when bouncing a user to the appropriate stage.

eawoods commented 6 years ago

Rubber band action is terrific!!! Also nice detail on the flash messages when selecting a step ahead.

Last thing, it's small and probably infrequent BUT...if you are on cross-list you can pass to upload without triggering warnings...but if you jump ahead FROM cross-list TO any other step than upload, the flash message will tell you that you have to fill out a cross-list before continuing.

Since we don't really want to encourage unnecessary cross-lists (as I understand it), that step wants to end up on upload for further-on steps, I think. It's not super clear from that point in the workflow, but if someone does that I think it will be better to land them on upload than on cross-list.

erickpeirson commented 6 years ago

Aha, good catch; will revise now

erickpeirson commented 6 years ago

Ok, try now