Skip to content

Proposed fix for Export Sketch feature which was not working.#2316

Merged
jaegeral merged 2 commits into
google:masterfrom
hkhalifa:fix-sketch-export
Sep 1, 2022
Merged

Proposed fix for Export Sketch feature which was not working.#2316
jaegeral merged 2 commits into
google:masterfrom
hkhalifa:fix-sketch-export

Conversation

@hkhalifa

@hkhalifa hkhalifa commented Sep 1, 2022

Copy link
Copy Markdown
Collaborator

Please provide enough information so that others can review your pull request:

This PR fixes a bug in the Sketch Export feature.

Checks

  • All tests succeed.
  • Unit tests added.
  • e2e tests added.
  • Documentation updated.

Closing issues
closes #2315

@hkhalifa

hkhalifa commented Sep 1, 2022

Copy link
Copy Markdown
Collaborator Author

Note that

query_filter = json.loads(view.query_filter)
indices = query_filter.get("indices", self.sketch_indices)

returned the wrong index. It was just a 1 digit int. So i replaced it with

indices = self.sketch_indices

Not sure if this is the best way to do it, and also could find the origin of the wrong index number in the query_filter. hence this is just a Proposal, and will be waiting for input regarding this.

@jaegeral jaegeral self-requested a review September 1, 2022 07:13

@jaegeral jaegeral left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thx for the fix

@jaegeral jaegeral merged commit 88750ad into google:master Sep 1, 2022
Comment on lines -350 to +352
indices = query_filter.get("indices", self.sketch_indices)
if not indices or "_all" in indices:
indices = self.sketch_indices
indices = self.sketch_indices

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know that this is already merged, but this block means that the saved search will be wrong in the case that the filter has specific indices selected, and will potentially return unexpected result, which is probably not what we want?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ping @jaegeral who reviewed this PR

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ya, i mentioned this in the comment above.

"Not sure if this is the best way to do it, and also could find the origin of the wrong index number in the query_filter. hence this is just a Proposal, and will be waiting for input regarding this."

@hkhalifa hkhalifa Sep 1, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is something wrong with

query_filter = json.loads(view.query_filter)
indices = query_filter.get("indices", self.sketch_indices)

Not sure where is the index in the query filter is set. it returned the wrong index. It was just a 1 digit int. If you have any pointers I can try debug this.

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.

Export Sketch feature returns HTTP 500 error

3 participants