CRM-19948: Store Attachment uploader information #11739

monishdeb · 2018-02-28T14:07:27Z

This PR changes the Attachment API in order for it to save the current logged in contact ID as the uploader of the file.

The ID is stored in the created_id field, which was added to the File entity. On Attachment.create, only when adding a new file (i.e. File.id is empty), we set the created_id as logged in contact ID.

The created_id field is not required and, on delete, it's set to null. This was done because:

There might be existent files on the database
A few other places use the File BAO directly and we don't want/need to update all them right now
When deleting a contact, we still might want to keep the file.

Note: I couldn't find a way to test the upgrader, so I'm not really sure if it's working. I tried to play with https://github.com/civicrm/civicrm-upgrade-test but didn't have any success

CRM-19948: Store the Attachment uploader information

monishdeb · 2018-02-28T14:15:17Z

ping @colemanw @totten @eileenmcnaughton

eileenmcnaughton · 2018-03-02T00:03:21Z

api/v3/Attachment.php

@@ -147,6 +147,7 @@ function civicrm_api3_attachment_create($params) {
  $fileDao->copyValues($file);
  if (!$id) {
    $fileDao->uri = CRM_Utils_File::makeFileName($name);
+    $fileDao->created_id = CRM_Core_Session::getLoggedInContactID();


I think it's better to do this on the spec function - ie
'api.default' => 'user_contact_id',

yeah right .. how could I overlooked it :( Will do it

eileenmcnaughton · 2018-03-02T00:08:29Z

@monishdeb I understand this PR is a reviewer's commit so if you have tested it I'm Ok with it - although I made one stylistic comment.

I think there is a pretty strong precedent for adding this sort of column and while adding columns to big tables should be done cautiously due to the length of time the upgrade script can take (we can't impose 1-2 hour outages every month) my feeling is that the file table is not likely to be massive even for large sites.

Regarding the upgrader test - you mean you couldn't just run the upgrade locally?

monishdeb · 2018-03-02T14:28:08Z

Thanks, @eileenmcnaughton for your feedback. I have made the additional fix, also earlier I tested the patch over a multilingual upgrade on my local. And the test build covers the monolingual upgrade, so I think the PR is safe to merge.

colemanw · 2018-03-02T16:04:51Z

@monishdeb I'm surprised the AllCoreTables.data.php file didn't get updated by codeGen to use square bracket syntax for arrays. Did you have to do any manual updating of the file or were there any issues with codeGen?

colemanw · 2018-03-02T19:16:16Z

@monishdeb I'm still very confused as to how the arrays managed to stay in the old syntax after being passed through codeGen the first time you did this, and why they are suddenly coming out in the correct syntax now, even though we haven't done any updates to codeGen.

monishdeb · 2018-03-02T19:34:34Z

@colemanw that's because the CodeGen will simply ignore if that file is already present (see here where CRM_Core_CodeGen_Reflection::needsUpdate() return TRUE if that files is not present and eventually hits the gencode). Reason why we were not getting the correct syntax. I have then deleted the CRM/Core/DAO/AllCoreTables.data.php and then executed the bin/regen.sh which corrected syntax. (Another alternative is to run sh bin/regen.sh -g)

totten · 2018-03-06T02:21:19Z

I'm still very confused as to how the arrays managed to stay in the old syntax after being passed through codeGen

that's because the CodeGen will simply ignore if that file is already present

This is incorrect. The Reflection::needsUpdate() check says that:

If the file does not exist, then it must be updated.
If the file does exist, then generate a quick preview ($this->getRaw()) and compare that to the actual code (using an an approximate equivalence test, isApproxPhpMatch(...)).

The problem here is that the equivalence-test (isApproxPhpMatch()) is too clever, owing to c3fc262#diff-ec73ce9b7d53e38552af8595f882ae9bR77 -- note how line 77 means that both $expected and $actual will be filtered through ArraySyntaxConverter before comparing them. In effect, that line says: "two PHP files are equivalent if all the code is the same, notwithstanding array-syntax variations".

I suppose there is a legitimate argument that the two files are truly equivalent (because array-syntax differences are trivial). But I can't think of a reason why we'd want to treat them as equivalent for GenCode purposes. We'd rather err towards having the current *.php match the current *.tpl. Just remove the call to ArraySyntaxConverter, run GenCode, and make sure you commit the current revisions of any generated files.

colemanw · 2018-03-06T14:15:28Z

@totten the reason for c3fc262 is that our old php beautifier is incapable of processing files with short array syntax. Also, the smarty function {print_array} does not seem capable of outputting short syntax. So when generating DAOs, the existing files will have short syntax and the generated will have old syntax. Hence the necessity to convert them before comparing as strings.

monishdeb · 2018-04-16T19:11:19Z

@colemanw @totten I have rebased this PR and shifted the upgrade code to 5.2alpha1. Please have a look!

colemanw · 2018-04-16T19:30:09Z

api/v3/Attachment.php

+    'title' => 'Created By Contact ID',
+    'type' => CRM_Utils_Type::T_INT,
+    'description' => 'FK to civicrm_contact, who uploaded this file',
+    'api.default' => 'user_contact_id',


I wonder if the File BAO is a more appropriate place to set this default value than the API? Setting it here will only affect apiV3.

monishdeb · 2018-04-18T12:28:02Z

CRM/Upgrade/Incremental/php/FiveTwo.php

+   *
+   * @param string $rev
+   */
+  public function upgrade_5_2_alpha1($rev) {


@eileenmcnaughton like I was saying if we need to set it to upgrade_5_2_alpha1

oh - will watch & see if @totten / @colemanw merge this :-)

colemanw · 2018-04-19T01:37:10Z

CRM/Core/BAO/File.php

+    $fileDAO = new CRM_Core_DAO_File();
+    $fileDAO->copyValues($params);
+
+    if (empty($params['created_id'])) {


I think this ought to also be conditional if there is no $params['id'] to avoid overwriting this value on update.

monishdeb · 2018-05-21T20:37:16Z

@colemanw @eileenmcnaughton can you please review & merge this PR?

(cherrypicked commits from #9761 + additional changes)

colemanw

I think this is good except for missing hooks.

colemanw · 2018-05-22T19:39:28Z

CRM/Core/BAO/File.php

+      $fileDAO->created_id = CRM_Core_Session::getLoggedInContactID();
+    }
+
+    $fileDAO->save();


I think this BAO::create() function needs to call pre/post hooks.

oops missed it, thanks for pointing that out.

Added the hook calls. Can you please check now?

eileenmcnaughton · 2018-05-24T22:12:35Z

@colemanw what does approved mean? How does that fit with 'good to merge'

colemanw · 2018-05-28T15:35:57Z

@eileenmcnaughton I had been about to merge it but then waned to verify with @totten that the upgrade code was going into the correct place. Got that question answered so now merging.

civicrm-builder added the master label Feb 28, 2018

monishdeb mentioned this pull request Feb 28, 2018

CRM-19948: Store Attachment uploader information #9761

Closed

monishdeb force-pushed the CRM-19948 branch 2 times, most recently from 2f9ec22 to 76e60f8 Compare February 28, 2018 14:13

monishdeb force-pushed the CRM-19948 branch 2 times, most recently from 65670f7 to a3f143e Compare February 28, 2018 18:42

eileenmcnaughton reviewed Mar 2, 2018

View reviewed changes

monishdeb force-pushed the CRM-19948 branch from a3f143e to 3ea405f Compare March 2, 2018 14:25

colemanw mentioned this pull request Mar 2, 2018

[NFC] Update AllCoreTables.data.php #11754

Closed

colemanw mentioned this pull request Mar 2, 2018

AllCoreTables - Generate file every time #11755

Merged

monishdeb force-pushed the CRM-19948 branch 4 times, most recently from d91c2d9 to c74ea2d Compare April 16, 2018 19:10

colemanw reviewed Apr 16, 2018

View reviewed changes

monishdeb force-pushed the CRM-19948 branch 2 times, most recently from a8f8085 to b9efc25 Compare April 18, 2018 12:04

monishdeb commented Apr 18, 2018

View reviewed changes

monishdeb force-pushed the CRM-19948 branch from b9efc25 to 4d7f650 Compare April 18, 2018 14:35

colemanw reviewed Apr 19, 2018

View reviewed changes

monishdeb force-pushed the CRM-19948 branch from 4d7f650 to db70368 Compare April 19, 2018 04:06

monishdeb force-pushed the CRM-19948 branch from db70368 to f1fd7c0 Compare April 28, 2018 08:52

monishdeb force-pushed the CRM-19948 branch 4 times, most recently from f598621 to 4914b37 Compare May 21, 2018 20:35

monishdeb force-pushed the CRM-19948 branch from 4914b37 to cefb4ac Compare May 22, 2018 04:09

monishdeb added the has-test label May 22, 2018

colemanw requested changes May 22, 2018

View reviewed changes

davialexandre and others added 3 commits May 23, 2018 01:12

CRM-19948: Add The created_id field to File

c3438d1

CRM-19948: Store the logged in contact ID as the file uploader

7aab005

set default created_id value to BAO

ae2c7e0

monishdeb force-pushed the CRM-19948 branch from cefb4ac to ae2c7e0 Compare May 22, 2018 19:46

colemanw approved these changes May 24, 2018

View reviewed changes

colemanw merged commit bf9ba88 into civicrm:master May 28, 2018

ahed-compucorp mentioned this pull request Jun 2, 2023

dev/core#4333 Set default created_id for case and activity Attachments #26409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-19948: Store Attachment uploader information #11739

CRM-19948: Store Attachment uploader information #11739

monishdeb commented Feb 28, 2018

monishdeb commented Feb 28, 2018

eileenmcnaughton Mar 2, 2018

monishdeb Mar 2, 2018

eileenmcnaughton commented Mar 2, 2018

monishdeb commented Mar 2, 2018

colemanw commented Mar 2, 2018

colemanw commented Mar 2, 2018

monishdeb commented Mar 2, 2018

totten commented Mar 6, 2018

colemanw commented Mar 6, 2018

monishdeb commented Apr 16, 2018

colemanw Apr 16, 2018

monishdeb Apr 18, 2018

eileenmcnaughton Apr 18, 2018

colemanw Apr 19, 2018

monishdeb commented May 21, 2018 •

edited

Loading

colemanw left a comment

colemanw May 22, 2018

monishdeb May 22, 2018

eileenmcnaughton commented May 24, 2018

colemanw commented May 28, 2018

CRM-19948: Store Attachment uploader information #11739

CRM-19948: Store Attachment uploader information #11739

Conversation

monishdeb commented Feb 28, 2018

monishdeb commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 2, 2018

monishdeb commented Mar 2, 2018

colemanw commented Mar 2, 2018

colemanw commented Mar 2, 2018

monishdeb commented Mar 2, 2018

totten commented Mar 6, 2018

colemanw commented Mar 6, 2018

monishdeb commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented May 21, 2018 • edited Loading

colemanw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 24, 2018

colemanw commented May 28, 2018

monishdeb commented May 21, 2018 •

edited

Loading