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

fix: handling of third-party wearable on ExtendedUrnParser class #6202

Merged
merged 7 commits into from
Jun 4, 2024

Conversation

aleortega
Copy link
Contributor

What does this PR change?

This PR addresses how third-party wearables are processed by the ExtendedUrnParser class. This class is responsible for shortening URNs when they are extended (i.e., urn.Split(':').Length > 6). When an extended URN is received, it cuts off the last part and returns the shortened version. This is necessary because the /entities/active endpoint from Catalysts' content servers expects the shortened URN to return the wearable/emote metadata.

The issue was that wearables from third-party providers contain 7 parts, causing the code to mistakenly treat them as extended URNs, which is not the case. Third-party wearables have an additional part but that part do not represent a token id (which is the part that should be removed), so we need to call the Catalysts endpoint using the entire URN received instead of shortening it.

The changes in this PR prevent the removal of the last part of the URN when this method receives a wearable's URN from a third-party provider (i.e., urn.Contains("collections-thirdparty")).

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

Copilot summary

copilot:summary

* Update protobuf generated classes from protocol. Added new avatar attach points and ids. Updated LoadingAvatar.prefab.

* Updated scripts/package.json to point to protocol branch with new avatar attach points

* Updated scripts/package.json to protocol@next

* Added even more avatar attach points
* bump @dcl/scene-runtime

* add comment and skip the adjustStackTrace in preview mode, fix package-lock.json

* update package
@aleortega
Copy link
Contributor Author

aleortega commented May 31, 2024

@aleortega aleortega added the do not merge This PR should not be merged because is experimental or for testing purposes. label May 31, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

I left a small comment and a question. Looks good to me but I have some concerns

@aleortega aleortega marked this pull request as draft June 3, 2024 14:12
@aleortega aleortega marked this pull request as ready for review June 3, 2024 16:54
@lorux0 lorux0 changed the base branch from dev to main June 4, 2024 15:41
@lorux0 lorux0 enabled auto-merge June 4, 2024 15:43
@lorux0 lorux0 disabled auto-merge June 4, 2024 15:58
@lorux0 lorux0 merged commit d3f77d6 into main Jun 4, 2024
12 checks passed
@lorux0 lorux0 deleted the fix/tp-wearable branch June 4, 2024 15:59
@LucasLioyQA
Copy link
Contributor

QA checked.

Please, check the following issue tracker to check which issues have been found and fixed while testing this PR and what still needs to be fixed>

https://docs.google.com/spreadsheets/d/19V7h30vyjgqmhKSjvISv7aSdIrfYxDEZ/edit#gid=1851350851

No third party emotes could be tested, since no one could be obtained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge This PR should not be merged because is experimental or for testing purposes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants