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

[#30] Improve event query filter #35

Merged
merged 23 commits into from
Dec 25, 2020

Conversation

daenamkim
Copy link
Collaborator

Part of #30

Screen Shot 2020-11-30 at 20 26 57

Copy link
Collaborator

@shoenseiwaso shoenseiwaso left a comment

Choose a reason for hiding this comment

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

I couldn't get this to work with the following:

image

Even after removing the check in my comment below. Happy to look at this together if and/or take some suggestions in case I'm missing something in my manual test setup!

@@ -166,7 +153,6 @@ function buildCustomQuery(events, groupBy, orderBy, limit, offset) {
// build the event query
for (let i = 1; i < events.length; i++) {
const event = events[i];

Copy link
Collaborator

Choose a reason for hiding this comment

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

The section above that validates the number of columns will need to be adjusted. The logic is still setup for when selects and filters were both of size 3.

What we can probably do is just remove this check altogether:

if ((header.length - 1) % 3 !== 0) {
  throw new Error(`Expecting number of columns to be divisible by 3 plus 1, found ${header.length} columns total`);
}

Since a more comprehensive check is already being done inside extractSelectFilterCounts().

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry. I found the same issue yesterday. I tested complicated filters and worked but simple one didn't work 😓 I will let you know once I fixed this part.

Copy link
Collaborator Author

@daenamkim daenamkim Dec 15, 2020

Choose a reason for hiding this comment

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

@shoenseiwaso I have fixed you mentioned and other expected issues. I have tried to test as much as I can but I will try to continue to find out something else. (I setup another environment which cloned but embedded Apps Script with a spreadsheet so you can test independently from now.)

image

FYI, data should be as follows
image

@shoenseiwaso shoenseiwaso self-assigned this Dec 13, 2020
@daenamkim
Copy link
Collaborator Author

Once your review is done, I will be happy that you review https://github.com/curvegrid/v2.curvegrid.com/pull/215 too 🙇

Copy link
Collaborator

@shoenseiwaso shoenseiwaso left a comment

Choose a reason for hiding this comment

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

LGTM! Worked as expected!

Comment on lines -56 to -62
const ruleParts = RegExp('^([A-Za-z._]+)([0-9]*)$').exec(rulePath[j]);
if (ruleParts === null || ruleParts.length < 2) {
throw new Error(`Invalid rule '${rulePath[j]}' in '${rules}'`);
}
const rule = ruleParts[1].toLowerCase();
if (rule === '') {
throw new Error(`Sub-rule is empty in '${rules}'`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much simpler than what we had, nice!

@daenamkim daenamkim merged commit 7cc7360 into 30-fix-event-queries-filters Dec 25, 2020
@daenamkim daenamkim deleted the 30-improve-event-query-filter branch December 25, 2020 06:53
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.

2 participants