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

Fix cardinality sub filter for rational periodic point filter #142 #158

Merged
merged 7 commits into from
Oct 10, 2024

Conversation

BradleySheldon
Copy link
Collaborator

@BradleySheldon BradleySheldon commented Sep 30, 2024

Fixes #142

What was changed?
The cardinality sub-filter under rational periodic points has been implemented.

Here, describe what part of the application you changed (e.g. login page, database, etc.). If possible, mention what specific components were added, removed, or modified.
Frontend: modified the filter handling for cp_cardinality so that the correct filter is passed from the UI.
Backend: Updated the database query logic to handle cp_cardinality correctly

Why was it changed?
This issue was changed so the users can more effectively filter the data they are searching for.

Here, describe the issue that you are fixing with this code, and why your code fixes it.
The cardinality filter under Rational Periodic Points was not functioning at all. It was returning all results regardless of the filter input. The fix is so that users can now filter this data based on the criteria.

How was it changed?
This code was changed by adjusting the filter logic of the Rational Periodic Point filter in both the frontend and backend as well as reviewing which database queries to implement.

Here, get into detail about what files you modified, and talk about the most important lines in regards to fixing the issue.
Added support to postgres_connector.py to filter by cp_cardinality in SQL queries. In FilterContext.js I added the cp_cardinality key to the filter context to pass the correct filter value to the backend. And in ExploreSystems.js the logic was modified to capture and send the cp_cardinality filter from the UI to the backend.

Remaining Issue:
Largest cycle is still not implemented, I could not find where this data was being stored to be implemented

Screenshots that show the changes (if applicable):

Copy link
Collaborator

@dracpak dracpak left a comment

Choose a reason for hiding this comment

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

The cardinality filter appears to work based off of the cp_cardinality. Largest cycle still needs to be implemented. Also, Cardinality and Largest Cycle should only ever be numbers, so consider changing the text box to only accept numbers to prevent user error and mitigate SQL Injection (although there are other places to inject)

Backend/postgres_connector.py Outdated Show resolved Hide resolved
Frontend/src/pages/ExploreSystems.js Outdated Show resolved Hide resolved
@suprajamannava17
Copy link
Collaborator

The cardinality filter is functioning as expected for numerical values, but allowing text input could lead to errors and security concerns. I recommend updating the input field to accept numbers only, as both fields should be numerical. While the filter logic for cardinality is in place, the largest cycle feature still needs to be implemented to fully meet the feature requirements. Great progress so far, but let’s focus on refining input validation and completing the largest cycle logic.

Copy link
Collaborator

@dracpak dracpak left a comment

Choose a reason for hiding this comment

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

Cardinality filter works however the statistics page breaks because it's trying to reference columns in the where text that don't exist. PR 157 has a fix for this if you just want to implement it quickly, although it also could be improved with a variable.

@suprajamannava17 suprajamannava17 self-requested a review October 9, 2024 22:13
@suprajamannava17
Copy link
Collaborator

All the filters are working fine

@suprajamannava17 suprajamannava17 merged commit e921f81 into main Oct 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

rational periodic point filter does not work
3 participants