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

[WIP] Save custom reports #1924

Closed
wants to merge 94 commits into from
Closed

Conversation

carkom
Copy link
Contributor

@carkom carkom commented Nov 17, 2023

I have a working prototype for saving custom reports. Rather than working behind closed doors, I wanted to get this out there so people can offer feedback and bug reports. It's working pretty well. I still have some tweaks to make. LMK what you think!

@Redbox3070 Redbox3070 mentioned this pull request Nov 18, 2023
39 tasks
@MikesGlitch

This comment was marked as duplicate.

empty INTEGER DEFAULT 0,
hidden INTEGER DEFAULT 0,
uncat INTEGER DEFAULT 0,
selectedCategories TEXT,
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏do we even need this? We already have conditions_op that can be used to filter specific categories. So perhaps we can remove the duplicate DB column?

In the UI we can keep it. But when the user selects to hide a few categories - it just adds a new filter to the view using conditions_op.

WDYT?

Copy link
Contributor Author

@carkom carkom Dec 12, 2023

Choose a reason for hiding this comment

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

I see your point. Implementing it in my mind seems complex. Maybe there's a better way to do it.

Eg. if you unselect one category you could use "is not" & categoryId. If you unselect all and just choose one you could use "is" & categoryId. When you start adding more and more to either side eventually they meet in the middle.

Or maybe you always use "is any" for field and op. I guess that could work, just thinking out loud. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it seems like it may be difficult to differentiate actual filters from "selected list". We could always make a new type that combines the two in one array.

Comment on lines 17 to 19
viewLegend INTEGER DEFAULT 0,
viewSummary INTEGER DEFAULT 0,
viewLabels INTEGER DEFAULT 0,
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏I'm a bit torn about these. From one perspective.. it makes sense to store them in the database. But then the user experience might be a bit jarring.

Let me explain.

Lets say you have 5x widgets with various viewSummary states. You open up one widget - it opens summary side panel. Then you open a different - it does NOT open summary side panel. Which to me feels a bit annoying. I would expect to have it open everywhere if I had it open before. So to use userPrefs for the global visibility of the summary side panel instead of a per-widget config.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could see an arguement to be made for either side so I guess I'm torn as well. I'd be fine with either implementation. Show/hide of all these elements is pretty quick so I guess I'd lean toward global variables like you suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to go with global variables I think.

@youngcw
Copy link
Member

youngcw commented Nov 21, 2023

How would saving reports work with the time options of "year to date" and "last year" would those get saved as raw date ranges and needed re created upon a new year or new month?

@carkom
Copy link
Contributor Author

carkom commented Nov 21, 2023

How would saving reports work with the time options of "year to date" and "last year" would those get saved as raw date ranges and needed re created upon a new year or new month?

When the report is saved start date is set to currentMonth.firstMonthThisYear and the end dat is set to current month. It's not a dynamicly changing variable that updates with each passing day.

I suppose it would be possible to update those dates each time the user opens the report.

@youngcw
Copy link
Member

youngcw commented Nov 21, 2023

When the report is saved start date is set to currentMonth.firstMonthThisYear and the end dat is set to current month. It's not a dynamicly changing variable that updates with each passing day.

I suppose it would be possible to update those dates each time the user opens the report.

I bet the "last year" option isn't a big deal since that wont change really at all, but the "year to date" one would be nice to have the new months get added.

@youngcw
Copy link
Member

youngcw commented Nov 21, 2023

If you do setup a way to save a "last year" report it would also be useful to have a this month and last month sliding window.

Maybe one option would be to have a few built in ranges that are saved as strings like "last year", "this year", "last month", "last 3 months", "this quarter" and such, then also have the option for a custom window that doesn't change.

@MatissJanis
Copy link
Member

Just linking this for visibility while we're on the topic of sliding windows: #1924 (comment)

@carkom
Copy link
Contributor Author

carkom commented Dec 12, 2023

@MatissJanis what are you thoughts on something like this?

Copy link
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jan 14, 2024
@carkom
Copy link
Contributor Author

carkom commented Jan 15, 2024

I should be able to pick this up again in another week. Just waiting on other PRs to get merged first.

@youngcw youngcw removed the Stale label Jan 15, 2024
@carkom
Copy link
Contributor Author

carkom commented Feb 5, 2024

This PR got too far behind master and became unmanageable to try to fix and bring current.

New PR is #2335

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.

5 participants