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.

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

[ConflictResolver] add support for JSON Instruments [ known issue ] #9248

Closed
christinerogers opened this issue May 9, 2024 · 4 comments · Fixed by #9270
Closed

[ConflictResolver] add support for JSON Instruments [ known issue ] #9248

christinerogers opened this issue May 9, 2024 · 4 comments · Fixed by #9270
Assignees
Labels
Category: Bug PR or issue that aims to report or fix a bug

Comments

@christinerogers
Copy link
Contributor

for the roadmap:

Update the conflict resolver to show conflicts on JSON instruments.

per C-Big testing for release 26, discussed may 9 with @ridz1208 and Dave

@christinerogers christinerogers added the Category: Bug PR or issue that aims to report or fix a bug label May 9, 2024
@driusan
Copy link
Collaborator

driusan commented May 10, 2024

I don't see anywhere in the code that this would break.

the conflict resolver uses NDB_BVL_Instrument::factory with the commentID and then calls _saveValues() to save the new entry:
https://github.com/aces/Loris/blob/main/modules/conflict_resolver/php/endpoints/unresolved.class.inc#L260-L273

The conflict detector uses the factory and calls diff:
https://github.com/aces/Loris/blob/main/php/libraries/ConflictDetector.class.inc#L78

and diff uses getInstanceData which is handles JSON data correctly:
https://github.com/aces/Loris/blob/main/php/libraries/NDB_BVL_Instrument.class.inc#L2064

The only possible problem is this line:
https://github.com/aces/Loris/blob/main/php/libraries/NDB_BVL_Instrument.class.inc#L2080

which depends on how the person coded the instrument. If they use jsonData and set table=testname in their linst file or php instrument it should work transparently as far as I can tell.

@ridz1208
Copy link
Collaborator

@driusan
Copy link
Collaborator

driusan commented May 10, 2024

That is the same line I referenced above and if it's the only problem this ticket is a duplicate of #9242

@driusan
Copy link
Collaborator

driusan commented May 16, 2024

@CamilleBeau I'm assigning this to you since you self-assigned #9242 and it should be the same fix. Just make sure your PR notes both as resolved.

@ridz1208 ridz1208 linked a pull request May 28, 2024 that will close this issue
1 task
@driusan driusan closed this as completed in 52ff079 Jun 3, 2024
maximemulder pushed a commit to maximemulder/Loris that referenced this issue Sep 25, 2024
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
Labels
Category: Bug PR or issue that aims to report or fix a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants