Skip to content

fix(discover1): Fix date for GlobalSelectionHeader not being updated#15948

Merged
leedongwei merged 1 commit into
masterfrom
dlee/ISSUE-637
Dec 9, 2019
Merged

fix(discover1): Fix date for GlobalSelectionHeader not being updated#15948
leedongwei merged 1 commit into
masterfrom
dlee/ISSUE-637

Conversation

@leedongwei

Copy link
Copy Markdown
Contributor

@lynnagara It's been >1 year since you looked at this but I wanted to check in to make sure I'm not missing anything. I removed the topSavedQuery on discover/sidebar/savedQueryList.tsx because the UX does not make sense to me.

Current implementation sorts the queries by dateUpdated but the most recently viewed query is placed at the top (and then moved back to its original position when another query is viewed). IMO if we want to pop and place the most recently viewed query on top of the stack, then we should be tracking a lastSeen property.

@dashed I can't tell if this is related to the issue that you're trying to fix in #15400. I'm not encountering any issues with the date format.

Refs ISSUE-637

@leedongwei leedongwei self-assigned this Dec 5, 2019
@lynnagara

Copy link
Copy Markdown
Member

Would you mind adding a screenshot / gif of what has changed? Percy is not showing any diff. I don't really have feedback on the UX side of things (this was @ckj / design team's area) but this should probably go through design review before merging.

Also, is the date fix separate to the new UX that is being introduced, or do they somehow depend on each other? I'd suggest splitting this into 2 PRs if possible, the PR title only really captures half of what's going on here.

Comment thread src/sentry/static/sentry/app/views/discover/index.tsx Outdated
Comment thread src/sentry/static/sentry/app/views/discover/sidebar/savedQueryList.tsx Outdated
Comment thread src/sentry/static/sentry/app/views/discover/index.tsx
Comment thread src/sentry/static/sentry/app/views/discover/index.tsx Outdated
@lynnagara

Copy link
Copy Markdown
Member

The test failures are related

@dashed

dashed commented Dec 5, 2019

Copy link
Copy Markdown
Member

@leedongwei It is likely related to my PR at #15400

I was able to still reproduce the issue I was resolving as follows:

  1. load a saved query with saved absolute date ranges
  2. using the start and end dates of the saved query in step 1 to perform an API call to retrieve results
  3. an error will be shown.

The crux is the issue is that after saving a Discover v1 query with absolute date ranges, the Django backend will return those same absolute date ranges in a different datetime format.

This datetime format is incompatible with the /api/0/organizations/sentry/discover/query/ endpoint.

Screen Shot 2019-12-05 at 6 06 31 AM

My approach in my PR was to normalize any and all datetime strings to a compatible datetime format on the frontend side.

@leedongwei

leedongwei commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

@lynnagara The changes are not UI-related.

For the list of SavedQueries:

Previously:

  1. We have data sorted by dateUpdated: [a, b, c, d]
  2. When we click on "c", it becomes: [c, a, b, d]
  3. Then we click on "b", it becomes: [b, a, c, d]
  4. Then we click on "d" and we update it: [d, a, b, c]

In this PR:

  1. We have data sorted by dateUpdated: [a, b, c, d]
  2. When we click on "c", it becomes: [a, b, c, d]
  3. Then we click on "b", it becomes: [a, b, c, d]
  4. Then we click on "d" and we update it: [d, a, b, c]

@lynnagara

Copy link
Copy Markdown
Member

I don't quite follow why we only update at d but not b or c. What's the rule here?

@dashed

dashed commented Dec 5, 2019

Copy link
Copy Markdown
Member

@leedongwei makes sense to me 👍

@lynnagara To clarify, the invariant is that the list is always sorted by dateUpdated.

Clicking on saved queries is not considered an action that updates a saved query.

@leedongwei

Copy link
Copy Markdown
Contributor Author

The original implementation was that the list was sorted by dateUpdated. When a query is viewed, it is moved to index 0. When another query is viewed, the previous query is moved to back to its original index (but I would expect it to be moved to index 1).

The PR removes this quirky movement (along with fixing a bug or two).

@dashed dashed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

@leedongwei leedongwei merged commit 3616d8f into master Dec 9, 2019
@leedongwei leedongwei deleted the dlee/ISSUE-637 branch December 9, 2019 23:11
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants