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

ref(tags): Fix custom tags issue #21281

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Conversation

priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Oct 12, 2020

closes: https://app.asana.com/0/1182975495223914/1197541196070295

Description: The UI or the event json is not showing the keys of the custom tags that were left empty or exceeded in the desired length.

it looks like below:

Chrome

image

Mozilla:

image

Safari:

image

@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2020

size-limit report

Path Size
public/app.js 239.1 KB (-0.01% 🔽)
public/vendor.js 445.86 KB (+0.01% 🔺)

@priscilawebdev priscilawebdev force-pushed the fix/fix-custom-tags-issue branch from be56b73 to 1bc33cb Compare October 13, 2020 07:16
@priscilawebdev priscilawebdev changed the title ref(tags): Fix custom tags issue - Part 1 ref(tags): Fix custom tags issue Oct 13, 2020
@priscilawebdev priscilawebdev requested review from jan-auer and a team October 13, 2020 09:27
for i, e in enumerate(tags)
if e.get("_meta")
}
tags_meta = prune_empty_keys({six.text_type(i): e.pop("_meta") for i, e in enumerate(tags)})
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a test for the Event serializer covering this fix.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, this was still missing in the PR. There are tests covering this, but the tests were incomplete. They are now complete.

@jan-auer
Copy link
Member

Further tests are not possible at the moment as they will require on an updated version of sentry-relay which can only be released after getsentry/relay#808, which needs to wait on getsentry/snuba#1419.

@priscilawebdev This still needs to account for null in tag keys. After updating to the above Relay PR, you can test with the following test event:

{
  "tags": {
    "a": "", 
    "": "b",
    "??": "c"
  }
}

This renders the following UI:

image

And also creates an error on the console:

Warning: Encountered two children with the same key, null.

Also, I'm 👍 to @markstory's suggestion to ship refactorings in a separate PR.

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

The frontend bits LGTM

@priscilawebdev priscilawebdev changed the title WIP: ref(tags): Fix custom tags issue ref(tags): Fix custom tags issue Oct 15, 2020
@priscilawebdev priscilawebdev force-pushed the fix/fix-custom-tags-issue branch from 5fd958e to bff99f9 Compare October 16, 2020 17:55
@priscilawebdev priscilawebdev force-pushed the fix/fix-custom-tags-issue branch from bff99f9 to f1388da Compare October 19, 2020 08:20
@priscilawebdev priscilawebdev changed the title ref(tags): Fix custom tags issue WIP:ref(tags): Fix custom tags issue Oct 19, 2020
@priscilawebdev priscilawebdev force-pushed the fix/fix-custom-tags-issue branch from f1388da to f342659 Compare October 19, 2020 10:27
@priscilawebdev priscilawebdev force-pushed the fix/fix-custom-tags-issue branch from f342659 to 6dd1ba8 Compare October 19, 2020 10:44
@priscilawebdev priscilawebdev changed the title WIP:ref(tags): Fix custom tags issue ref(tags): Fix custom tags issue Oct 19, 2020
@priscilawebdev priscilawebdev merged commit 7ab7256 into master Oct 19, 2020
@priscilawebdev priscilawebdev deleted the fix/fix-custom-tags-issue branch October 19, 2020 15:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 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