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

Fixed cascade logic for dropdown options filtering #30

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

brandonzhu09
Copy link
Contributor

@brandonzhu09 brandonzhu09 commented Jul 20, 2024

Description

Bugfix for improper cascading logic when listing the valid dropdown options in the data viewer. Resolved this issue by applying the correct number of filters at each update option event in the backend.

Resolves issue GEOS-ESM/evagram_input#9.

image

As shown in the image, there are no longer groups that do not belong to the variable as well as the owner, experiment, and observation associated with it.

@asewnath
Copy link
Collaborator

Hi Brandon, I'm working on testing the updates to evagram and I've found that I'm able to view an experiment name even though I haven't selected a User. Are you also getting this behavior?

Screenshot 2024-07-22 at 10 57 17 AM

@brandonzhu09
Copy link
Contributor Author

Hi Brandon, I'm working on testing the updates to evagram and I've found that I'm able to view an experiment name even though I haven't selected a User. Are you also getting this behavior?

Screenshot 2024-07-22 at 10 57 17 AM

Yes, I just noticed that I have been getting this behavior as well, but I found where the issue was in the initial load of the page and have fixed the bug.

@asewnath
Copy link
Collaborator

After killing pg4admin processes I had on port 5432 and then running docker-compose.yaml (my local database is now able to use 5432:5432) and then running evagram-input, I'm able to see the new input data. Bugfix works on my end. Excellent work!

Copy link
Collaborator

@tariqhamzeyssai tariqhamzeyssai left a comment

Choose a reason for hiding this comment

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

A few overall comments that may not be immediately relevant to the bugfix:

  1. First of all, great work on fixing this bug!
  2. Please add """ docstrings to every function.
  3. Some of the functions here are helpers, not "views" themselves. Namely everything from get_owners and beyond. Would it be appropriate to import them from a new util module? I want to keep the views as minimal as possible.

@brandonzhu09
Copy link
Contributor Author

A few overall comments that may not be immediately relevant to the bugfix:

  1. First of all, great work on fixing this bug!
  2. Please add """ docstrings to every function.
  3. Some of the functions here are helpers, not "views" themselves. Namely everything from get_owners and beyond. Would it be appropriate to import them from a new util module? I want to keep the views as minimal as possible.

That sounds like a good idea! Where would the util module go in our file structure?

@tariqhamzeyssai
Copy link
Collaborator

tariqhamzeyssai commented Jul 30, 2024

@brandonzhu09 Should it go in src/evagram/website/backend/?

@brandonzhu09 brandonzhu09 merged commit 86fb723 into develop Jul 30, 2024
2 checks passed
@brandonzhu09 brandonzhu09 deleted the bugfix/cascade_options branch July 30, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants