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

[Login] Summary Statistics #9518

Merged
merged 21 commits into from
Feb 12, 2025
Merged

[Login] Summary Statistics #9518

merged 21 commits into from
Feb 12, 2025

Conversation

skarya22
Copy link
Contributor

@skarya22 skarya22 commented Dec 20, 2024 •
edited
Loading

Brief summary of changes

  • Added a customizable summary of LORIS statistics to the Login page for guest users to see
  • A new table was created that is populated with the counts of pinned dataquery queries, and separated by project
  • This table is read by the login module for projects with showSummaryOnLogin as true in the Project database table.
  • If no data is populated, then the summary is not shown on the login page
  • Data is populated by the new tool: tools/update_login_summary_statistics.php. It must be run for the data in the summary to be updated. Can be added to a project's nightly cron, or at each release
  • Some queries are not necessarily possible in the DQT, so in-order to keep it customizable for projects, I added the ability to add SQL files to a folder in project/tools/Login_Summary_Statistics that can be added to the summary list.
  • There is a clipboard icon at the bottom right where if you press it, your clipboard will be filled with a csv version of the data
image

Testing instructions (if applicable)

  1. Run the patch and the script, then open the login page and see that the default queries from SQL/Login_Summary_Statistics have appeared.
  2. Login as admin, and go to the dataquery module. Create a query that includes the column Project. Run the query then go back to the dataquery homepage and pin the query to the login page. Give it a name, and leave out the 's' for plural (for example, Male Participant instead of Male Participants).
  3. Run the script, then logout and see that the summary appears with a dropdown of all projects. When you select a different project, the count should change.
  4. Change a project to showSummaryOnLogin = FALSE in the Project SQL table.
  5. Re-run the script and note that the project is no longer in the dropdown, and the 'All Projects' count has decreased.
  6. Try creating a new query that you only want to show for one project. To do this, create the query, including the Project column, then add a filter for Project = 'Pumpernickel'. Pin the project as before.
  7. Run the script, and open the login page. See that the query is not included in the All Projects section, nor in other projects, and is only shown in Pumpernickel.
  8. Now, try adding a query such as this to project/tools/Login_Summary_Statistics and see that the default queries have disappeared as it is now overridden.
    01_Site.txt

CCNA OVERRIDE PR

@skarya22 skarya22 added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Dec 20, 2024
@skarya22
Copy link
Contributor Author

The configuration for the summary statistics is currently done in two ways:

  1. Pin a query from the datadict module to the Login Page
  2. Add an SQL query to the project/tools/Login_Summary_statistics folder.

I had really liked the datadict integration, but after finishing I tried to test out some queries that we have on CCNA and realized they weren't possible. For instance, getting the distinct amount of sites with sessions for each project. I then added the ability to add SQL patches, which is a lot more simple.

This PR needs discussion on whether I should just stick with the SQL files, or also keep the ability to pin queries from the DQT before I continue work on this.

@skarya22 skarya22 added the State: Needs work PR awaiting additional work by the author to proceed label Jan 3, 2025
@skarya22 skarya22 self-assigned this Jan 3, 2025
@skarya22 skarya22 added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs work PR awaiting additional work by the author to proceed State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed labels Jan 3, 2025
@skarya22
Copy link
Contributor Author

skarya22 commented Jan 7, 2025 •
edited
Loading

From loris meeting Jan 7:

  • Create a list of all the generalized statistics for LORIS core and ask each project to confirm that the values are correct for what they want
  • Create a way to export data / download / script to export?
  • Keep both implementations

@driusan driusan mentioned this pull request Jan 21, 2025
Closed
@skarya22 skarya22 added Category: Feature PR or issue that aims to introduce a new feature Priority: High PR or issue should be prioritised over others for review and testing Language: PHP PR or issue that update PHP code Language: Javascript PR or issue that update Javascript code Project: CCNA Issue or PR related to the CCNA project Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Module: login PR or issue related to login module and removed State: Needs work PR awaiting additional work by the author to proceed labels Jan 23, 2025
@driusan driusan requested a review from GeorgeMurad February 3, 2025 20:07
GeorgeMurad
GeorgeMurad approved these changes Feb 5, 2025
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 reviewed Feb 7, 2025
View reviewed changes
modules/login/jsx/summaryStatistics.js
color: '#104985',
}}
>
{statistic.Value.toLocaleString('fr-CA')}{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this hardcoded fr-CA for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for showing numbers as: 100 000 instead of 100000

driusan
driusan reviewed Feb 7, 2025
View reviewed changes
modules/login/jsx/summaryStatistics.js Outdated
* @author Saagar Arya
* @version 1.0.0
*/
class SummaryStatistics extends Component {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not going to block over it but I don't think it's a good idea to be making new class based components.

driusan
driusan reviewed Feb 7, 2025
View reviewed changes
SQL/New_patches/2024-12-18-Login_statistics.sql Outdated
@@ -0,0 +1,16 @@
DROP TABLE IF EXISTS Login_Summary_Statistics;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't drop tables in release patches. If something needs to be dropped (doesn't seem to be since it's created below), it should go in a cleanup patch.

driusan
driusan reviewed Feb 7, 2025
View reviewed changes
SQL/New_patches/2024-12-18-Login_statistics.sql Outdated
ALTER TABLE Project
ADD COLUMN showSummaryOnLogin BOOLEAN DEFAULT TRUE;

UPDATE Project SET showSummaryOnLogin = FALSE WHERE Name = 'DCP';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the show showSummaryOnLogin column is for projects to be able to opt-out from being displayed on the login page, and then DCP is added as an example of how to do that in raisinbread

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this patch is going to the release patch, not raisinbread. Can you remove this RB specific line?

driusan
driusan requested changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

see comments

@skarya22 skarya22 self-assigned this Feb 7, 2025
@skarya22 skarya22 requested a review from driusan February 11, 2025 16:04
@skarya22 skarya22 assigned driusan and unassigned skarya22 and GeorgeMurad Feb 11, 2025
@skarya22 skarya22 added this to the 27.0.0 milestone Feb 11, 2025
driusan
driusan requested changes Feb 12, 2025
View reviewed changes
SQL/New_patches/2024-12-18-Login_statistics.sql Outdated
ALTER TABLE Project
ADD COLUMN showSummaryOnLogin BOOLEAN DEFAULT TRUE;

UPDATE Project SET showSummaryOnLogin = FALSE WHERE Name = 'DCP';
Copy link
Collaborator

Choose a reason for hiding this comment

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

But this patch is going to the release patch, not raisinbread. Can you remove this RB specific line?

@skarya22 skarya22 requested a review from driusan February 12, 2025 20:53
@driusan driusan merged commit 1e706f2 into aces:main Feb 12, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kongtiaowang kongtiaowang kongtiaowang left review comments

@GeorgeMurad GeorgeMurad GeorgeMurad approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@driusan driusan

Labels
Category: Feature PR or issue that aims to introduce a new feature Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Language: Javascript PR or issue that update Javascript code Language: PHP PR or issue that update PHP code Module: login PR or issue related to login module Priority: High PR or issue should be prioritised over others for review and testing Project: CCNA Issue or PR related to the CCNA project
Projects
None yet
Milestone
27.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@skarya22 @driusan @kongtiaowang @GeorgeMurad