icatproject / icat.server

The ICAT server offering both SOAP and "RESTlike" interfaces to a metadata catalog.
Other
1 stars 5 forks source link

Add InvestigationFacilityCycle relationship #263

Closed tomhayter closed 2 years ago

tomhayter commented 2 years ago

Adds a many-to-many relationship between Investigation and Facility Cycle by creating an InvestigationFacilityCycle table, as proposed in #259. Forgive me if there is anything I have missed, this is my first time working on the icat project.

@RKrahl has added some changes related to changing the order of SQL statements and fixing a Foreign Key name bug, however I'm not sure if this was intended to be on this branch?

closes #259

kevinphippsstfc commented 2 years ago

I have reviewed and tested Tom's changes. I found a few problems so corrected those and made a few improvements whilst I was reviewing and testing. I'm now happy with the changes and will approve the PR.

@RKrahl as Tom mentioned, I don't know where the first two commits by you come from. I can see them listed under the 'Commits' tab but when I view the 'Files changed' tab I don't see those same changes listed there. Any ideas?

RKrahl commented 2 years ago

Now, I get errors from the tests:

Tests run: 16, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.075 sec <<< FAILURE! - in org.icatproject.core.manager.TestEntityInfo
getters(org.icatproject.core.manager.TestEntityInfo)  Time elapsed: 0.004 sec  <<< FAILURE!
java.lang.AssertionError: Investigation count expected:<29> but was:<30>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.icatproject.core.manager.TestEntityInfo.testGetters(TestEntityInfo.java:448)
        at org.icatproject.core.manager.TestEntityInfo.getters(TestEntityInfo.java:389)

setters(org.icatproject.core.manager.TestEntityInfo)  Time elapsed: 0 sec  <<< FAILURE!
java.lang.AssertionError: Investigation count expected:<25> but was:<26>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.icatproject.core.manager.TestEntityInfo.testSetters(TestEntityInfo.java:471)
        at org.icatproject.core.manager.TestEntityInfo.setters(TestEntityInfo.java:401)

The version at 32634282 was fine, so apparently something went wrong with your merge of icat-schema-extension into 259_investigation_facility_cycle in be283604. (In case your are interested in details: both branches, icat-schema-extension and 259_investigation_facility_cycle independently increased the some numbers in TestEntityInfo.getters() and TestEntityInfo.setters(). Unfortunately, they happened to increase them to the same value, so git merge assumed that change from 259_investigation_facility_cycle to be already present in icat-schema-extension and ignored it. But the increase from both sides would have need to be taken cumulative … Similar issue also in TestWS.)

I'd suggest, I just drop the defective merge commit, e.g. force push 32634282 into 259_investigation_facility_cycle and merge the result directly into icat-schema-extension. Hope you don't mind.

tomhayter commented 2 years ago

That sounds like a good plan Rolf, cheers for sorting this. I fixed the merge conflicts on GitHub, which explains why it didn't pick up the difference in TestEntityInfo.gettters() and TestEntityInfo.setters()

RKrahl commented 2 years ago

That sounds like a good plan Rolf, cheers for sorting this. I fixed the merge conflicts on GitHub, which explains why it didn't pick up the difference in TestEntityInfo.gettters() and TestEntityInfo.setters()

Actually, you wouldn't have noticed it doing the merge with the git command line either. The problem was that both branches made the same change, resulting in the same line of code. This goes beyond the capabilities of git to understand that the result of the merge needs to be different from what both source branches agree on.

I needed to redo the same defective merge (using git command line) myself ending up again with a code that wouldn't build in order to understand what happened.