Tv4g1 issue1843 publishing cardinality

[spficklin]

Bug Fix

Issue #1843

Tripal Version: 4.x

Description

This PR solves the issue described in issue #1843 where field values derived from linked tables in Chado were only showing the first item and ignoring the others. In actuality the problem was that the testing we did made it look like it was publishing the first item but in fact it was publishing the wrong thing but because record IDs were similar between the base record and linked record we didnt notice. For example. If you add a project and link two contacts to it, then publishin the contacts on the project, it looked like it only added one, but it was really publishing a contact with the same record ID as the project not the contact. Since the first contact was what was desired it looked like it was working .

There were multiple issues that this PR solves in both ChadoStorage and TripalPublish:

1. There was a bug introduced that prevented title tokens from being renamed. I fixed that with the help of @dsenalik and @laceysanderson
2. The ChadoStorage::findValues() function sets the field property values after it finds all of the fields. One property that all fields have is the record_id which always points to the base record. When we find values, we always include the fields in the values array passed in because we want to find any field items to publish. This resulted in the findValues function setting the record_id for the first item for all fields regardless if any items were found. This resulted in publishing of incomplete items. They didn't seem to show up on the pages, but they were in the Drupal tables. This was fixed by adding a new function named ChadoStorage::isFieldValid() that the findValues() uses to make sure a field has all of the required values before including it in what is returned. I also add a has_values to the ChadoRecords array that is set if the values for the item are really set. This allows the isFieldValid() to really be sure that an item has a set value or not (i.e. empty values are sometimes okay). Also, the findValues() function was presetting the IDs and Links on linked tables and it should not have been doing that.
3. The reason that we could publish all properties but not all contacts on a project was because the ChadoRecords::findRecords() function was not looking at all of the ancialliary tables. There was a bug in that it exepcted that at least on of the columns in the linked table had to be used. This is the case in a property table where the value column is used, but in the case of a linking table, we don't need any other columns but the IDs. So, once the problem in item 2 above was fixed, no project_contact records were being published at all. I fixed this so the findRecords() function now will go through all linked tables. This is fixed.
4. The TripalPublish::insertFieldItems() had a bug in that it wasn't properly cycling through the entities correctly. This is fixed. I also fixed the way the number of published field items was reported.

Testing?

1. Load a brand new site.
2. Create an analysis
3. Create an organism
4. Create 2 or more projects
5. Create 2 or more contacts
6. Add a lot of records using the GFF loader and publish content from it. This tests that normal publishing is still working
7. Link the contacts to projects with this SQL statement: insert into project_contact (project_id, contact_id) values (1,2), (1,3), (2,2), (2,3);
8. Now publish the project. It should add two contact items to each of the two project pages.

[spficklin]

I have not yet added any testing, but this is working so I thought I'd open it up for functional testing and code review.

[dsenalik]

I build a new docker and created two contacts and five projects (so that I could make sure id's did not overlap)

 project_id |                  name                  |                                        description                                                                      
------------+----------------------------------------+-----------------------------------------------------------------------------------------
          1 | Colossus - The Forbin Project          | <p>Do not turn it on</p>
          2 | The Tripal Project                     | <p>Cool Stuff</p>
          3 | Manhattan Project                      | <p>dangerous</p>
          4 | Advanced LIGO                          | <p>A large-scale physics experiment and observatory to measure gravitational waves</p>
          5 | Superconducting Super Collider Project | <p>Cancelled</p>
(5 rows)

insert into project_contact (project_id, contact_id) values (4,2), (4,3), (5,2), (5,3);
Then published project:

 [notice]   Publishing 7 items for field: project_analysis...
ERROR: SQLSTATE[23505]: Unique violation: 7 ERROR:  duplicate key value violates unique constraint "tripal_entity__project_analysis____pkey"
DETAIL:  Key (entity_id, deleted, delta, langcode)=(6, 0, 0, und) already exists.: 
      INSERT INTO "tripal_entity__project_analysis"

etc. but this was using the wrong branch, let's try again

[spficklin]

Thanks @dsenalik for the quick check! Hmmm looks like something is still wrong. I'll see what's up. I'm also seeing a functional test fail.

[dsenalik]

OOPS! I used the wrong branch, from PR #1867 🙈 - building a new docker
On the branch in this PR publishing is working as expected.

[dsenalik]

There is something wrong with publishing an organism. I create it manually with
insert into organism (genus, species) values ('Tripalus', 'ficklinus');

Then when I publish
(debugging shows that this happens on the infraspecific_name column)
...
[notice] Step 2 of 6: Generate page titles...
[warning] Undefined array key 0 TripalPublish.php:479
[warning] Trying to access array offset on null TripalPublish.php:479
[warning] Trying to access array offset on null TripalPublish.php:479
[error] Error: Call to a member function getValue() on null in Drupal\tripal\Services\TripalPublish->getEntityTitles() (line 479 of /var/www/drupal/web/modules/contrib/tripal/tripal/src/Services/TripalPublish.php) #0 /var/www/drupal/web/modules/contrib/tripal/tripal/src/Services/TripalPublish.php(870): Drupal\tripal\Services\TripalPublish->getEntityTitles(Array)
#1 /var/www/drupal/web/modules/contrib/tripal/tripal/src/api/tripal.publish.api.php(35): Drupal\tripal\Services\TripalPublish->publish()
#2 [internal function]: tripal_publish('organism', 'chado_storage', Array, Object(Drupal\tripal\Services\TripalJob))
#3 /var/www/drupal/web/modules/contrib/tripal/tripal/src/Services/TripalJob.php(395): call_user_func_array('tripal_publish', Array)
... and so on...

[dsenalik]

There is another problem 😢
Create an organism in chado
insert into organism (genus, species, common_name, comment) values ('Tripalus', 'ficklinuus', 'Ficklins Tripal', 'A rare blue variant of the common tripal');
Publish organism
Published 1 new entities, and 6 field values.
Rebuild cache to be safe
View it - looks okay.

Enter a second organism
insert into organism (genus, species, common_name, comment) values ('Tripalus', 'senalikus', 'Senaliks Tripal', 'A rare purple variant of the common tripal');
Publish organism
Published 1 new entities, and 6 field values.
Rebuild cache to be safe
The page only has a title and no other fields.

If you try to edit, everything is blank, including genus and species.
The drupal tables don't contain anything for this entity_id, only the previous one is there (explaining the blank edit form)

[dsenalik]

This review is based on including my last two suggestions, and testing with publishing two organisms one at a time as described in comment #1882 (comment) now works correctly.

I tested elsewhere more thoroughly, everything appears to be working as intended.
In addition to project-contact, I linked two publications to analysis and these published correctly.
GFF importing and publishing gene, mRNA worked also.

[spficklin]

Thanks @dsenalik for the testing! I've integrated all of your suggestions. I'm working on functional tests right now, so hopefully will have some of that soon.

[dsenalik]

I have tested everything I had tried previously, and everything is working as expected.
I do have a number of typos that I spotted, but otherwise this is a super-awesome PR!

[spficklin]

Thanks @dsenalik for the additional review! It looks like your mispelling fixes got merged in. I just approved the one where you made a change to the regex.

@laceysanderson in answer to this question:

One big question though: I see you used the bootstrap theme in testing 😬 Why did you end up going that route rather then using stark as recommended by Tripal?

I don't know :-) Something I brought in accidentally and it has been removed.

[laceysanderson]

Code review suggestions. The suggestions in the test are improvements only. I may have found a bug in chadorecord changes though.

[laceysanderson]

It might be good to add a comment here why you didn't use setColumnValue so it doesn't get changed back in the future 🙈

[laceysanderson]

Thanks for all the changes @spficklin, this looks good to merge to me!

[laceysanderson]

Merging this since we have 2 reviews and passing tests! Thanks @spficklin for fixing this pesky one!!!