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

Refactor: Revert filter workarounds #437

Closed
bmtcril opened this issue Sep 27, 2023 · 8 comments
Closed

Refactor: Revert filter workarounds #437

bmtcril opened this issue Sep 27, 2023 · 8 comments
Labels
enhancement Relates to new features or improvements to existing features stale

Comments

@bmtcril
Copy link
Contributor

bmtcril commented Sep 27, 2023

Due to bugs in Superset 2.1 we had to change our whole filtering strategy to prevent courses and blocks from courses that instructors didn't have access to being displayed as filter choices. This should be fixed in Superset 3.0 so we should revert those changes:

  • Course name should be required any place where charts won't show without it
  • Problem name / Video name should be required any place where charts won't show without them
  • Any places where we short circuit virtual datasets based on not having a filter chosen should be removed, it this means the virtual dataset can be moved back to dbt we should do that

#377 most changes came from this PR, so it should be a good guide for what needs to be undone.

@bmtcril bmtcril changed the title fix: Revert filter workarounds Refactor: Revert filter workarounds Sep 27, 2023
@bmtcril bmtcril added the enhancement Relates to new features or improvements to existing features label Sep 27, 2023
@SoryRawyer
Copy link
Contributor

Maybe this is a caching issue, but I'm still seeing incorrect values for filters when enabling "select first value by default" (e.g. seeing demo course problem names despite having the UMONTREAL org selected). Could someone else confirm if they're seeing this behavior as well?

@bmtcril
Copy link
Contributor Author

bmtcril commented Oct 11, 2023

@SoryRawyer I'm not seeing it on the beta site, either as instructor or admin. I've changed the filters in the -en instructor dashboard to make course name required and "select first filter value by default". Could be a data ordering issue (which course was loaded first?), but can you want to try it out on the beta site and let me know what you think?

@bmtcril
Copy link
Contributor Author

bmtcril commented Oct 11, 2023

You may also want to make sure you've got the right version of the Superset image as there were some bumps for me upgrading locally to Superset 3. The image should be around edunext/aspects-superset:0.52.2 or so.

@SoryRawyer
Copy link
Contributor

SoryRawyer commented Oct 12, 2023

Interesting. I'm seeing the correct behavior on the beta server, however I also briefly see "2 options" under the course run filter, followed quickly by a change to "1 option". When I make course run a required filter locally, I get a non-UMONTREAL course run as the default value and a non-UMONTREAL problem/video name as the default value. It seems like even with course run as an optional filter, I see a non-UMONTREAL problem/video name. If you make problem/video names required on the beta server, does that behavior come up?

@bmtcril
Copy link
Contributor Author

bmtcril commented Oct 12, 2023

I've configured the "-en" dashboard on the beta server to make the problem and video names required and still can't replicate the behavior, which is maddening. I'm not too worried about the quick-change while filters load, but am curious what you see now on the beta.

@SoryRawyer
Copy link
Contributor

SoryRawyer commented Oct 12, 2023

Huh, this is what I see when I look at the problem performance tab:
Screenshot from 2023-10-12 11-05-56

Similarly, I see "A shared culture" as the default video when I look at the video performance tab.

@bmtcril
Copy link
Contributor Author

bmtcril commented Oct 12, 2023

Looks like this isn't fixed after all, we should punt on it for now. I'll mark this as stale so we don't lose it.

@bmtcril bmtcril added the stale label Oct 12, 2023
@bmtcril
Copy link
Contributor Author

bmtcril commented Jan 12, 2024

This is going to be obviated by changes for the v1 dashboards based on the new PRD.

@bmtcril bmtcril closed this as completed Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Relates to new features or improvements to existing features stale
Projects
None yet
Development

No branches or pull requests

2 participants