Skip to content

Add InvestigationFacilityCycle relationship#263

Merged
RKrahl merged 16 commits intoicat-schema-extensionfrom
259_investigation_facility_cycle
May 18, 2022
Merged

Add InvestigationFacilityCycle relationship#263
RKrahl merged 16 commits intoicat-schema-extensionfrom
259_investigation_facility_cycle

Conversation

@tomhayter
Copy link
Copy Markdown
Contributor

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.

  • I have run the icat.server tests and they pass, as well as doing some of my own testing using python-icat.
  • I have also included the necessary SQL statements to update both MySQL and Oracle databases.

@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

@tomhayter tomhayter added the schema this involves changes to the ICAT schema label Jan 19, 2022
@kevinphippsstfc kevinphippsstfc added this to the 5.0.0 milestone Feb 4, 2022
@kevinphippsstfc
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member

@RKrahl RKrahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just one minor comment. Otherwise this looks good for me.

@RKrahl
Copy link
Copy Markdown
Member

RKrahl commented May 18, 2022

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 3263428 was fine, so apparently something went wrong with your merge of icat-schema-extension into 259_investigation_facility_cycle in be28360. (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 3263428 into 259_investigation_facility_cycle and merge the result directly into icat-schema-extension. Hope you don't mind.

@tomhayter
Copy link
Copy Markdown
Contributor Author

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 RKrahl force-pushed the 259_investigation_facility_cycle branch from be28360 to 3263428 Compare May 18, 2022 11:15
@RKrahl RKrahl merged commit 8c7a7b2 into icat-schema-extension May 18, 2022
@RKrahl RKrahl deleted the 259_investigation_facility_cycle branch May 18, 2022 12:33
@RKrahl
Copy link
Copy Markdown
Member

RKrahl commented May 18, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

schema this involves changes to the ICAT schema

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a direct relationship between FacilityCycle and Investigation

3 participants