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

[Statistics] (20.0) Re-enable Behavioural Statistics Double Data Entry module #3752

Merged
merged 4 commits into from
Jun 21, 2018
Merged

[Statistics] (20.0) Re-enable Behavioural Statistics Double Data Entry module #3752

merged 4 commits into from
Jun 21, 2018

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Jun 20, 2018

Summary

This pull request addresses the issues raised in Redmine 14744. I believe many are related to some malfunctioning in the Raisinbread test database and are not an issue with the source code. Similarly, some of these issues seem to occur on the test VM but I am not able to reproduce them on my sandbox.

However, major issues existed preventing DDE functionality from working at all.

Details

behavioural stats: Candidate links to instrument forms don’t work. Takes you to instrument "page", and not the candidate instrument form page (404: Page not found)

  • /instruments/ was added to the Candidate links in the menu statistics site templates, restoring their functionality.

behavioural stats: Don’t have access to either “Click here for breakdown per participant” links even though I have all the permissions

  • I was able to replicate this bug for Double Data Entry but it was related to the below bug. I did not have this issue with the first link in the module (tried on both my VM and testing).

All ‘View Details’ links for Double Data Entry break [getSetting(‘statistics’) always returns null]

  • The cause was that Statistics_DD_Site was overriding its parent's parent's parent's __construct function resulting in none of the essential NDB_Page things being loaded (such as form, Module, Identifier, etc.)
  • imaging stats tab doesn't work
  • Occurs on test VM only

Other bonus changes

There was also no reason for its parent (Statistics_Site) to extend NDB_Form so it now extends NDB_Menu as the page is more like a Menu than a Form. This is reflected in the other template files in this module being prefixed with menu_ rather than form_.

I also fixed a redundancy in the function notexlcluded as well as several PHP warnings which occurred when $key was false.

getCssDependencies was removed as it was identical to its parent method and so does not need to be overridden.

To test

On branch 20.0-release

  1. Go to the Statistics module. Click the Behavioural Statistics Tab

  2. Scroll down until the Double Data Entry section. Click any View Details link.

  3. Verify you get a 500 error.

  4. Go back and, under DDE, "Click here for breakdown per participant". Another 500 should appear.

  5. Go to the main module page and click Imaging Statistics tab. It should work. If not, tell me.

  6. If you are on an instance with real instruments please check if the candidate links to instruments work. Let me know if they still break. If they work, the problem is Raisinbread and not LORIS.

On my branch

  • Do the first 4 steps again. The links should take you to the properly loaded module.

@johnsaigle johnsaigle added Category: Bug PR or issue that aims to report or fix a bug Cleanup PR or issue introducing/requiring at least one clean-up operation [branch] major labels Jun 20, 2018
@johnsaigle johnsaigle requested a review from zaliqarosli June 20, 2018 17:56
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

😄👯😄👯😄👍👍👍👍👍

@johnsaigle johnsaigle added this to the 20.0.0 milestone Jun 20, 2018
@driusan
Copy link
Collaborator

driusan commented Jun 20, 2018

@zaliqarosli Did you test this or just review it?

@johnsaigle
Copy link
Contributor Author

johnsaigle commented Jun 20, 2018

@driusan @zaliqarosli helped me make the changes in the first place -- we did most of this together.

Not sure if she tested it explicitly on her VM though

@zaliqarosli zaliqarosli added the Passed manual tests PR has been successfully tested by at least one peer label Jun 20, 2018
@zaliqarosli
Copy link
Contributor

zaliqarosli commented Jun 20, 2018

just tested it! if i helped make these changes, should I not also be reviewing it? because i approve lol

@johnsaigle
Copy link
Contributor Author

@zaliqarosli I am removing the testing label since this PR is attempting to do more than it originally was when you tested. Specifically, someone with functioning instruments needs to test the bugs you found. See the (updated) PR description for more info.

@johnsaigle johnsaigle removed the Passed manual tests PR has been successfully tested by at least one peer label Jun 20, 2018
@driusan driusan merged commit d8012a4 into aces:20.0-release Jun 21, 2018
@johnsaigle johnsaigle deleted the 180620-StatsFix branch January 23, 2019 17:57
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 Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants