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(feed): Parse MusicAudio and Video posts metadata on Map Feed #1400

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sujal-into
Copy link
Collaborator

fix: #1370

@sujal-into sujal-into added the Social Feed Everything related to the Social Feed label Nov 20, 2024
@sujal-into sujal-into self-assigned this Nov 20, 2024
Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit 8a0ebc6
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/674012b1d2f53c0008a99b70
😎 Deploy Preview https://deploy-preview-1400--testitori.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit 8a0ebc6
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/674012b1ba4a5800080d1ffe
😎 Deploy Preview https://deploy-preview-1400--teritori-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

const metadata = zodTryParseJSON(
ZodSocialFeedPostMetadata,
ZodSocialFeedPostMetadata.partial().merge(
ZodSocialFeedPostMetadata.pick({ title: true }),
Copy link
Collaborator

@WaDadidou WaDadidou Nov 20, 2024

Choose a reason for hiding this comment

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

You don't need to modify the Zod type to try parse.
Yes, ZodSocialFeedPostMetadata shape can be wrong, but we have made other shapes for other posts categories. So we need to handle them.

Reformulation: The issue is due to the fact we have 4 differents types for the fetched posts.
Here we only try to parse ZodSocialFeedPostMetadata. So, the tracks and videos posts validation fails.

The result here has no success: https://github.com/TERITORI/teritori-dapp/blob/bugfix/video-audio-map/packages/utils/sanitize.ts#L22
If you console.log the result, you will see what's wrong to validate ZodSocialFeedPostMetadata

To avoid that, because the map gathers all posts categories, we must handle these Zod types. Here is my suggestion:

const videoPostMetadata = zodTryParseJSON(
  ZodSocialFeedVideoMetadata,
  post.metadata,
);
const trackMetadata = zodTryParseJSON(
  ZodSocialFeedTrackMetadata,
  post.metadata,
);
const articleMetadata = zodTryParseJSON(
  ZodSocialFeedArticleMetadata,
  post.metadata,
);
const postMetadata = zodTryParseJSON(
  ZodSocialFeedPostMetadata,
  post.metadata,
);
const metadataToUse = videoPostMetadata || trackMetadata || articleMetadata || postMetadata;

We need a refacto about it. Either unify all types in one, either add an utilitary function to handle (try parse) these differents zod types

Copy link
Collaborator

@n0izn0iz n0izn0iz Nov 21, 2024

Choose a reason for hiding this comment

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

actually no there is the zodSocialFeedCommonMetadata schema when you need to only parse the title

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made a fix for this discussion in this commit: 3a16113

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually no there is the zodSocialFeedCommonMetadata schema when you need to only parse the title

In this case, there are other things that i need to parse also based on the category type, so I have created a util function to handle those cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can switch on the post type to do the proper parsing in each case, the whole point of using zod is to have type safety, using a z.ZodTypeAny entirely defeat this purpose

Copy link
Collaborator Author

@sujal-into sujal-into Nov 21, 2024

Choose a reason for hiding this comment

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

I have used switch and removed the ZodTypeAny : ece2bac

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

no z.ZodTypeAny please

Comment on lines 144 to 153
switch (category) {
case PostCategory.Video:
return zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata);
case PostCategory.Article:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata);
case PostCategory.MusicAudio:
return zodTryParseJSON(ZodSocialFeedTrackMetadata, metadata);
default:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
switch (category) {
case PostCategory.Video:
return zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata);
case PostCategory.Article:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata);
case PostCategory.MusicAudio:
return zodTryParseJSON(ZodSocialFeedTrackMetadata, metadata);
default:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata);
}
switch (category) {
case PostCategory.Video:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) || zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata);
case PostCategory.Article:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) || zodTryParseJSON(ZodSocialFeedArticleMetadata, metadata);
case PostCategory.MusicAudio:
return zodTryParseJSON(ZodSocialFeedTrackMetadata, metadata);
default:
return zodTryParseJSON(ZodSocialFeedPostMetadata, metadata);
}

We'll make another PR to replace all zodTryParseJSON usages by parseSocialFeedMetadata (About posts)

Copy link
Collaborator

@n0izn0iz n0izn0iz Nov 21, 2024

Choose a reason for hiding this comment

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

Hanlde the "old" posts

actually we will reset the feed after redesigning the contracts so I don't think it's important to support old stuff right now

even "We'll make another PR to replace all zodTryParseJSON usages by parseSocialFeedMetadata (About posts)" is not necessary since we'll have well defined types from contracts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made changes here: 8a0ebc6

Copy link
Collaborator

Choose a reason for hiding this comment

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

@n0izn0iz Yes, some issues are "stand by" for now

@WaDadidou WaDadidou changed the title fix: made keys for social post optional while parsing except title fix(feed): Parse MusicAudio and Video posts metadata on Map Feed Nov 21, 2024
Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

if you need other cases, add them, there should be one zod schema for one post category and the fallback is common metadata

packages/utils/types/feed.ts Outdated Show resolved Hide resolved
packages/utils/types/feed.ts Show resolved Hide resolved
Comment on lines +146 to +149
return (
zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) ||
zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return (
zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) ||
zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata)
);
return (
zodTryParseJSON(ZodSocialFeedVideoMetadata, metadata)
);

Comment on lines +151 to +154
return (
zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) ||
zodTryParseJSON(ZodSocialFeedArticleMetadata, metadata)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return (
zodTryParseJSON(ZodSocialFeedPostMetadata, metadata) ||
zodTryParseJSON(ZodSocialFeedArticleMetadata, metadata)
);
return (
zodTryParseJSON(ZodSocialFeedArticleMetadata, metadata)
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Social Feed Everything related to the Social Feed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(feed): MusicAudio and Video don't appear on the Map
3 participants