ericvaandering / DocDB

Working repository for DocDB
25 stars 18 forks source link

don't allow preserve sign option for changing docs or adding files ... #80

Closed lauramengel closed 6 years ago

lauramengel commented 6 years ago

if the user is only in the group to allow changing meta-data with preserving signatures.

If I am in the group that is allowed preserve only on changing meta-data, but not on changing the document and not on adding files - (i.e. I am in the group in @HackPreserveSignoffGroups, but not in the group in @HackDocsPreserveSignoffGroups) -

then I should not be given the message or checkbox to preserve data if I go to change the document or add files. Here's the message: (which is correct for changing meta-data, but not for changing the doc or adding files)

Warning: You are about to clear all the signatures on the document, unless you check the box (near the bottom of this page) to preserve signatures.

and the preserve checkbox does appear at the bottom of the page.

lauramengel commented 6 years ago

If I am in the needed @hackdocspreservesignoffgroups, but not in @hackpreservesignoffgroups, I don't get the option to preserve on a metadata change.

i.e. I can preserve signatures when I change a document or add files, but not when I change meta-data.

This may be a feature in that you can control them separately and we can just tell people they need to be in both hack groups to be able to preserve signoff while changing metadata, documents and adding files.

So I am fine with this. Just letting you know what it does since it sounded like you may have thought otherwise earlier.

lauramengel commented 6 years ago

Other problem: If I am in the @hackdocspreservesignoffgroups, and I go to change a document, it gives me the right message that I can preserve signatures if I check the preserve check box and it shows the preserve checkbox.

However when I change the document and check the preserve checkbox and submit, I get the error: "There were fatal errors processing your request: Signatures may only be preserved when modifying document meta-data." and the document is not changed.

Interestingly, the same thing works perfectly when I go to add files to the document, which we put in the same category as changing the document (as opposed to changing meta-data).

I am able to check preserve signoffs and the files are added, the signatures are not cleared, no emails are sent to signers to approve. If I choose not to check the preserve signoff box for add files, that also works as expected, adding the files and clearing the signatures.

ericvaandering commented 6 years ago

FYI, the setting for add files and change metadata is one thing and the document update another:

https://github.com/ericvaandering/DocDB/blob/8_8_9_br/DocDB/cgi/DocDBGlobals.pm#L145

I'll take another look at what you're seeing and sort out the issues.

lauramengel commented 6 years ago

Hi. Yes, that is the way it works in production now. A long while back you proposed (and I agreed):

On 12/27/17 11:16 AM, Eric W Vaandering wrote:

On this one issue of preserving signatures. It looks to me like the current behavior allows a person with the permissions to preserve signatures to also add files without removing the signatures. If we are splitting this into two permissions, it seems to me that it makes more sense to put the adding files permission with changing the document, not changing the metadata. What do you think?

Laura: A) If a person can change actual docs without triggering signatures, they should also be able to change meta-data without triggering signatures.

Eric: Only A makes sense. I don’t really have a way to check that on a full doc replacement, the meta-data remained constant.

On Dec 5, 2017, at 4:14 PM, Laura M. Mengel lauram@fnal.gov wrote: Yes, only one checkbox whether to preserve signatures on a form.

ericvaandering commented 6 years ago

Ok. Either the comments in the code are wrong or that got totally dropped. Makes sense.

Sent from a mobile device. Please excuse my brevity or transcription errors.

On Jun 17, 2018, at 12:30, lauramengel notifications@github.com<mailto:notifications@github.com> wrote:

Hi. Yes, that is the way it works in production now. A long while back you proposed (and I agreed):

On 12/27/17 11:16 AM, Eric W Vaandering wrote:

On this one issue of preserving signatures. It looks to me like the current behavior allows a person with the permissions to preserve signatures to also add files without removing the signatures. If we are splitting this into two permissions, it seems to me that it makes more sense to put the adding files permission with changing the document, not changing the metadata. What do you think?

Laura: A) If a person can change actual docs without triggering signatures, they should also be able to change meta-data without triggering signatures.

Eric: Only A makes sense. I don’t really have a way to check that on a full doc replacement, the meta-data remained constant.

On Dec 5, 2017, at 4:14 PM, Laura M. Mengel lauram@fnal.govmailto:lauram@fnal.gov wrote: Yes, only one checkbox whether to preserve signatures on a form.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ericvaandering_DocDB_issues_80-23issuecomment-2D397893813&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=EHaoB-POFWGrYFvPXoj1bQ&m=KFbZ9-GlxUp11cDM_kvqd7YU41l5E3_nFkLH1fjjAbM&s=kUfGdQYAoy8o3RoK3xvfDP86PwvxY4VJuntp3qmEWKU&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ABmErsNDt7fBOk32TjjYB-2DoK1sGw0d-5FTks5t9pIngaJpZM4UqJzQ&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=EHaoB-POFWGrYFvPXoj1bQ&m=KFbZ9-GlxUp11cDM_kvqd7YU41l5E3_nFkLH1fjjAbM&s=jimVEmlsowKqzkV27YTbjSIWaH27g8uwNqZDBEL7DeI&e=.

ericvaandering commented 6 years ago

@lauramengel I found a logic error that probably explains all of this. I fixed it. I also saw that the preserve box would appear on documents with no signatures requested which would just confuse things, so I think I changed that behavior too. Go ahead and test this too.

Since we're getting close I did another rebase, so the FNAL_SSO branch has everything but you'll need to do a reset to get it. Double check that the switching ordered text field still works (#54)

lauramengel commented 6 years ago

Still seeing some issues:

1) same as before: If I am in the @hackdocspreservesignoffgroups, and I go to change a document, it gives me the right message that I can preserve signatures if I check the preserve check box and it shows the preserve checkbox.

However when I change the document and check the preserve checkbox and submit, I get the error: "There were fatal errors processing your request: Signatures may only be preserved when modifying document meta-data." and the document is not changed.

2) When I click on "Add Files", I get Software Error: Undefined subroutine &main::RevisionStatus called at /web/sites/d/docdbdev.fnal.gov/cgi-bin/sites/DUNE_SSO/AddFilesForm line 102.

These items were fixed and passed tests

ericvaandering commented 6 years ago
  1. The rare case where a fix is removing code. The prohibition on any modification except meta data was still there. I added a note saying the permissions depend on the type of modification.

  2. Just a missing "require"

Both fixed by #82.

Again, I've rebased FNAL_sso onto the latest fixes.

lauramengel commented 6 years ago

prior issues re-tested and working except: when preserve signoffs upon changing a document, the new version of the document displays as an unmanaged document.

i.e. No "Signoffs:" heading listed near bottom. (and therefor, no signatures listed, no "remove signature" button displayed for me as I had approved the prior version.) Signatures were preserved on the prior version, but not copied to the new version made due to changing a doc.

ericvaandering commented 6 years ago

Ok, I think I found the problem. The old document was not really being looked up. I also added some debugging.

Try again using the branch "fix_sigcopy". Hopefully that works. If not, please send me the debug messages.

lauramengel commented 6 years ago

When using fix_sigcopy, if I try to create a document from 1 local file on my computer, I get: (I tried with both cert version and SSO version.

Software error: Undefined subroutine &main::RevisionStatus called at /web/sites/d/docdbdev.fnal.gov/cgi-bin/sites/DUNE_Cert_Req/DocumentAddForm line 259.

ericvaandering commented 6 years ago

Pull and try again. I had to move a require statement.

lauramengel commented 6 years ago

I am able to create a doc from a local file successfully now.

Still same issue changing a document with preserved signatures. The signatures are preserved on the initial version, but on the new version docb page, the "Signoffs" heading is not there (nor the previous signer that already approved it (with a button for them to remove signature).

Will look for debug info

lauramengel commented 6 years ago

Results for changing doc with preserve signatures I started clean by making a new version of a doc with 1 signoff (me) and I approved it. (using SSO DocDB version and my SSO account). When I change the document from a.txt to b,txt, some excerpts: (One of them says "Don't understand method to insert file with key 2")

The document's signatures have been identically preserved. Any changes in the list have been ignored.

You were successful. Your Document ID is DUNE-doc-2935, version 2. From Database DocID: 2935 From Database DRI: 24499 DI: 2935 V: 1 Finding EmailUserID by FNAL SSO name lauram@fnal.gov Determined user ID to be 6792 User explicity has groups 65 66 67 91 After SSO groups, DocDB groups for user: 65, 66, 67, 91, 65, 66 Final unique DocDB groups for user: 67, 66, 91, 65

From Database DRI: DI: 2935 V: 2
Copying signatures from DRI to 24502 with T/F 1
Adding files for DRI 24502
Trying to upload 1 b.txt Main: 1
Don't understand method to insert file with key 2
From Database DocID: 2935
From Database DRI: 24502 DI: 2935 V: 2
lauramengel commented 6 years ago

My copy of docdb-fixcopy branch from a pull at 4:09. So I think I got your most recent?

ericvaandering commented 6 years ago

4:03 on my end. The problem is still this:

Copying signatures from DRI to 24502 with T/F 1

There should be a number after DRI. I've got an idea.

lauramengel commented 6 years ago

confirmed it's the latest because if I pull again it says "already up to date"

ericvaandering commented 6 years ago

OK. Give it one more(?) try.

lauramengel commented 6 years ago

ok, starting ...

lauramengel commented 6 years ago

Do I need to start from scratch or can I just do a git pull?

ericvaandering commented 6 years ago

Just a pull.

lauramengel commented 6 years ago

Yes, that worked!! Did same as before (made new clean doc with 1 signature and approved. and the changed the document with preserve.

Now the signature/approval is preserved on both v1 and v2 and both have a button the remove signature.

Now debugging excerpts say: (Is this msg a concern? "Don't understand method to insert file with key 2")

From Database DocID: 2937
From Database DRI: 24504 DI: 2937 V: 1
...
After limiting, user belongs to unique groups 67, 66, 91, 65
Set OldRevID to 24504 in update mode
Copying signatures from DRI 24504 to 24507 with T/F 1
Adding a signoff to DRI 24507
Added a signoff: 1344
Copying a signature for 6792 to SI 1344: T/F 1 at 2018-06-18 16:44:31 
Transferring dependency 1344: to 1344:0
Adding files for DRI 24507
Trying to upload 1 b.txt Main: 1
Don't understand method to insert file with key 2
From Database DocID: 2937
From Database DRI: 24507 DI: 2937 V: 2
Finding EmailUserID by FNAL SSO name lauram@fnal.gov
Determined user ID to be 6792
lauramengel commented 6 years ago

Is there any reason I need to test this separately for CERT vs SSO? (Using Cert DocDB vs using SSO DocDB, also using cert account vs using SSO account - the permutations get mind boggling)

Is the code the same?

lauramengel commented 6 years ago

Also, might there be a different if the doc had multiple signatures instead of just 1 and/or if the document was not approved (or does it just copy whatever is there)?

Thanks!

ericvaandering commented 6 years ago

Code is the same. I'm going to rearrange the commits so that this is included in the latest FNAL_SSO branch. Will be done before I head home tonight.

I'm guessing the file method warning is harmless. I think it comes from the 2nd file box on the page that you are probably leaving empty. I had to look it up.

ericvaandering commented 6 years ago

It copies whatever is there and the same code is used for copying signatures from one version to another without preserving the state and the metadata update, but a test with two won't hurt. :-) There is a second part of the process that is copying the relationships (a before b) of the signatures.

ericvaandering commented 6 years ago

Ok, that's done. You probably want to reset back to the FNAL_SSO branch.

lauramengel commented 6 years ago

Thanks. Will do. Yes, I left the 2nd file box on the page empty.

lauramengel commented 6 years ago

If I am in HackDocsPreserveSignoff and I click to change meta-data, I am not given the option to preserve data. (check if user in HackDocsPreserveSignOff or HackPreserveSignoff for meta-data preserve?)

If I click change document or add files, I am given the option the preserve data.

ericvaandering commented 6 years ago

Do you get a warning message? Which one. You should get one of

Warning: You are about to modify a managed document. or Warning: You are about to clear all the signatures on the document, unless ...

lauramengel commented 6 years ago

I get: Warning: You are about to modify a managed document. This will clear all the signatures on the document.

ericvaandering commented 6 years ago

I found a mistake and added some debugging. That should take care of it. Sorry for the mess with this feature.

I'm going to fix the other one too and then prepare a new FNAL_SSO

lauramengel commented 6 years ago

If you give me 20 min, I think I can finish the whole set and let you know if I see anything else before you cut another.

lauramengel commented 6 years ago

Maye we should punt on user only in hackdocs not able to preserve when click on change metadata. I'd rather avoid anything that's thorny at this point.

If this is the only issue, we could tell users they have to be in both hack groups to preserve signatures for both meta-data changes and document/addfiles changes. That is not much of an inconvenience.

If they are in hackdocs, they'll be able to preserve when changing documents and doing addfiles. When they change a document, they'll also be able to change meta-data too as part of that transaction. (but it's easy enough to add the user to both hack groups, so they can do all 3.)

If they are just in hack, they'll only get to preserve when they change meta-data.

ericvaandering commented 6 years ago

It’s the applying for groups thing that is thornier than I thought. The preserve signatures was a stupid bug on my part.

lauramengel commented 6 years ago

ok. nevermind then.

lauramengel commented 6 years ago

Hi. The rest of the restest succeeded. So only change needed is allowing user HackDocsPreserveSignoffs to be able to use the Change Meta-Data link and get option to preserve signoffs.

(besides adding account, add groups button for new SSO user with no groups/ldap/cert.

lauramengel commented 6 years ago

Notes and questions:

i.e. Do I need re-test everything again, or just the metadata case for user in HackDocsPreserveSignoff?

3) Also, what is the probability that yesterdays and todays changes would affect

Do I need to check these again? These should clear signatures and do everything as before the preserve signoff feature was added.

lauramengel commented 6 years ago

Please go ahead and make the new version.

ericvaandering commented 6 years ago

Would it matter if one had signed and one had not? (since copying is copying?)

It should not.

The "Last Signed" field in the gray doc info box on the left shows the time of the first signature instead of the second signature. I'd rather leave this alone and note it for next time.

Agreed. Want to open an issue and target 8.8.10?

Is the change so that "HackDocsPreserveSignoff allows user to use change meta-data link and be given preserve option" very localized?

The code is very localized causing a bug in the code to determine if the preserve option is presented and able to be used. I wouldn't test every bit of the matrix, but a few things here and there.

3 [...]

Zero probability I would say.

New tag on FNAL_SSO is ready

lauramengel commented 6 years ago

Hi. being in hackdocssignoffs and changing metadata worked. other preservesignoff tests worked too. So we are in good shape. Back in an hour.

lauramengel commented 6 years ago

Reviewed my results and all were successful (had done all tests except when preserve signoff not available or not chosen, since those have worked each past trial and you mentioned these should not be affected.)

Closing Issue.

I believe this is our new release. Rob has been testing as well and found no problems.

Thank you!!

ericvaandering commented 6 years ago

OK. Let me reassemble the last few commits in a nicer way and consolidate down to one branch. We can still fix any last minute errors.