Fixes T7159.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T7159: Require users to sign the Phacility TOS before using the service
- Commits
- Restricted Diffusion Commit
rPd39da529ca26: Legalpad - allow for legalpad documents to be required to be signed for using…
Created a legalpad document that needed a signature and I was required to sign it no matter what page I hit. Signed it and things worked! Added a new legalpad document and I had to sign again!
Ran unit tests and they passed!
Logged out as a user who was roadblocked into signing a bunch of stuff and it worked!
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- T7159
- Lint
Lint Passed Severity Location Code Message Advice src/applications/legalpad/editor/LegalpadDocumentEditor.php:106 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 4484 Build 4498: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
For the update, two approaches:
- Just issue an UPDATE. As long as you're doing a raw UPDATE rather than loading/saving each session, this should be "fast enough" even for large instances, I think (I'd expect it to take a few seconds for 100K's of rows, probably). We could push it into the daemons after we get like 1M active sessions at a time or something, but that's a long time away.
- You could do a worker and do it page-by-page, but I don't think anyone will need that for a very long time and we'd get a cleaner result there if we wait for T5166 anyway.
Obviously, I'm in favor of UPDATE for now.
| src/applications/auth/storage/PhabricatorAuthSession.php | ||
|---|---|---|
| 43–45 | This isn't in the .sql and doesn't seem to be used? | |
| src/applications/base/controller/PhabricatorController.php | ||
| 56–58 | I figured the signature controller would need it, but we could just drop it if delegating worked fine. | |
| src/applications/legalpad/controller/LegalpadDocumentEditController.php | ||
| 144–151 | We should probably only allow administrators to set this flag, since it could be wildly disruptive. It should not be possible to set this flag if the signature type is "corporations" instead of "individausl". | |
| src/applications/legalpad/query/LegalpadDocumentQuery.php | ||
| 213–217 | Consider !== null and = %d so withSignatureRequired(false) excludes these documents. | |
| src/applications/auth/engine/PhabricatorAuthSessionEngine.php | ||
|---|---|---|
| 591–595 | Maybe we should only record this log if there were documents? | |
| src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php | ||
|---|---|---|
| 258 ↗ | (On Diff #28353) | whoops will remove |
Comment Actions
Quibbling inline.
| src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php | ||
|---|---|---|
| 258 ↗ | (On Diff #28353) | debugging code |
| src/applications/legalpad/controller/LegalpadDocumentEditController.php | ||
| 136 | Can't I still select "SIGNATURE_TYPE_CORPORATION" here? I don't see a transaction validation block anywhere. Maybe show the checkbox but disable it for non-admins? | |
Comment Actions
Oh, also: can I log out if I have to sign docs? Maybe that's something that we'd need noncompliantblahblah() for.
Comment Actions
- add back allowLegallyNonCompliantBlahblah
- use this where we use "allow partial sessions"
- show the checkbox but disabled for non-admins to "require signage"
- add some logic to the edit controller to prevent non-admins, and the type corp require sign error