Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[conflict_resolver] Rename TableName to TestName #9270

Merged
merged 10 commits into from
Jun 3, 2024
Merged

[conflict_resolver] Rename TableName to TestName #9270

merged 10 commits into from
Jun 3, 2024

Conversation

CamilleBeau
Copy link
Contributor

@CamilleBeau CamilleBeau commented May 16, 2024 •
edited
Loading

Brief summary of changes

  • Have you updated related documentation?

Testing instructions (if applicable)

  1. Try loading conflict resolver including an instrument that has a different test name / table name
  2. Try resolving conflicts
  3. Run the recreate_conflicts.php script, make sure there are no failures and conflicts are properly created
  4. Check that the dashboard properly displays the number of conflicts
  5. Test the conflicts tab of the Behavioural QC module to make sure it loads properly & links work
  6. Double check that there are no other places in the codebase where the change should be made

Link(s) to related issue(s)

@CamilleBeau CamilleBeau changed the title 2024 05 16 conflict resolver table name change [conflict_resolver] Rename TableName to TestName May 16, 2024
@CamilleBeau CamilleBeau force-pushed the 2024_05_16_conflict_resolver_TableName_Change branch from 654a847 to 3d39c25 Compare May 16, 2024 14:51
driusan
driusan reviewed May 16, 2024
View reviewed changes
php/libraries/NDB_BVL_Instrument.class.inc
@@ -2077,7 +2077,7 @@ abstract class NDB_BVL_Instrument extends NDB_Page
if (!in_array($key, $this->_doubleDataEntryDiffIgnoreColumns)) {
if ($otherData[$key] != $value) {
$diff[] = [
'TableName' => $this->table,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line is the only one that is required for the bug fix. The rest is just to make the column name "right" but involves changing the default schema, and regenerating the release patch very late in the release testing.

I think we should just change $this->table to $this->testName here for this release, and make the rest of the SQL changes (which are just renaming the column and the code to go with it) in the next release.

@CamilleBeau CamilleBeau force-pushed the 2024_05_16_conflict_resolver_TableName_Change branch from b954df0 to 9c72739 Compare May 16, 2024 15:47
@CamilleBeau
Copy link
Contributor Author

I was having some trouble with the raisinbread exporting, will try again later

@kongtiaowang
Copy link
Contributor

"raisinbread/RB_files/RB_conflicts_resolved.sql" and "raisinbread/RB_files/RB_conflicts_unresolved.sql" need to modify as well.

@ridz1208
Copy link
Collaborator

@driusan I'm partial to a full change, I can regenerate the patch its a very small addition and I really hate the idea of having a column table name storing the testname cause it will be forgotten and confusing I'm sure. lets just get it done properly now

@CamilleBeau CamilleBeau marked this pull request as ready for review May 23, 2024 14:25
kongtiaowang
kongtiaowang reviewed May 27, 2024
View reviewed changes
test/unittests/NDB_BVL_Instrument_Test.php Outdated
@@ -1624,7 +1624,7 @@ function testDiff()
$this->_instrument->diff($otherInstrument),
[
[
'TableName' => 'medical_history',
'TestName' => 'medical_history',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change 'medical_history' to 'Test' after "php/libraries/NDB_BVL_Instrument.class.inc" diff function changed.

@ridz1208 ridz1208 linked an issue May 28, 2024 that may be closed by this pull request
[ConflictResolver] add support for JSON Instruments [ known issue ] #9248
Closed
@ridz1208 ridz1208 mentioned this pull request May 28, 2024
Closed
GeorgeMurad
GeorgeMurad reviewed May 30, 2024
View reviewed changes
Copy link
Contributor

@GeorgeMurad GeorgeMurad left a comment

Choose a reason for hiding this comment

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

LGTM

@driusan driusan merged commit 52ff079 into aces:main Jun 3, 2024
28 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Jun 6, 2024
maximemulder pushed a commit to maximemulder/Loris that referenced this pull request Sep 25, 2024
@CamilleBeau @maximemulder
[conflict_resolver] Rename TableName to TestName (aces#9270)
029ecb9 
Properly refer to the column used to load instruments in the conflict resolver as "TestName" instead of "TableName" which matches how the argument passed to NDB_BVL_Instrument uses the argument.

Fixes aces#9242
Fixes aces#9248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@kongtiaowang kongtiaowang kongtiaowang left review comments

@GeorgeMurad GeorgeMurad GeorgeMurad left review comments

Assignees

@GeorgeMurad GeorgeMurad

Labels
None yet
Projects
None yet
Milestone
26.0.0
5 participants
@CamilleBeau @kongtiaowang @ridz1208 @driusan @GeorgeMurad