Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

[C-2879] Add validation to single track upload flow #3855

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

Kyle-Shanks
Copy link
Contributor

Description

  • Adding validation to the single track upload flow using the track metadata schema from sdk.
  • Some work needs to be done to update the schema in sdk and then import it here, but it should be an easy hot swap.
  • There was a lot of work here to try to bridge the track metadata types between the sdk and common, but open to suggestions on how to clean up some of the checks and data marshalling.

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Lots of manual testing

How will this change be monitored?

N/A

Feature Flags

N/A

@Kyle-Shanks Kyle-Shanks requested review from amendelsohn, dylanjeffers and a team August 7, 2023 19:18
Copy link
Contributor

@amendelsohn amendelsohn left a comment

Choose a reason for hiding this comment

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

Dang dude. Excellent work! This is a real minefield.

We should chat with @sliptype re: types in sdk. Maybe we can align a bit better and get rid of some of the ts-ignores?

Comment on lines +73 to +79
mood: z.optional(z.boolean()),
tags: z.optional(z.boolean()),
genre: z.optional(z.boolean()),
share: z.optional(z.boolean()),
playCount: z.optional(z.boolean()),
remixes: z.optional(z.boolean())
Copy link
Contributor

Choose a reason for hiding this comment

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

huh thought these were required but fieldVisibility is optional


const createTrackMetadataSchema = () => {
return createUploadTrackMetadataSchema()
.merge(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it is possible. Very cool! Annoying that we have to do this though

])
),
releaseDate: z.optional(
z.date().max(new Date(), { message: 'should not be in the future' })
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be a bug here where we generate the 'now' time when we create the schema and then we use a fresh moment() later for the input value. However, I think this is not a problem since we dayFloor the selected date. We should check that this not an issue for the default value.


export const EditPageNew = (props: EditPageProps) => {
const { tracks, setTracks, onContinue } = props

// @ts-ignore - Slight differences in the sdk vs common track metadata types
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to scope this down to just the release date line, or is it multiple things?
Just don't want the ts-ignore hiding other future errors

Copy link
Contributor Author

@Kyle-Shanks Kyle-Shanks Aug 7, 2023

Choose a reason for hiding this comment

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

yup i can check 👍

@@ -24,10 +24,13 @@ import { processFiles } from '../store/utils/processFiles'
import styles from './SourceFilesModalForm.module.css'
import { useTrackField } from './utils'

// NOTE: SDK uses camel casing for the fields within download
Copy link
Contributor

Choose a reason for hiding this comment

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

dang that's annoying. Maybe we can align these?

@@ -107,13 +120,13 @@ export const SourceFilesModalForm = () => {
icon={<IconSourceFiles className={styles.titleIcon} />}
preview={preview}
>
<SourceFilesModalFiels />
<SourceFilesModalFields />
Copy link
Contributor

Choose a reason for hiding this comment

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

lol whoops

@Kyle-Shanks Kyle-Shanks force-pushed the kj-Add-validation-to-single-track-upload-flow branch from db1287a to 1bc82ee Compare August 8, 2023 16:05
@Kyle-Shanks Kyle-Shanks force-pushed the kj-Add-validation-to-single-track-upload-flow branch from 1bc82ee to 4da85df Compare August 8, 2023 16:29
@Kyle-Shanks Kyle-Shanks merged commit 7f79830 into main Aug 8, 2023
@Kyle-Shanks Kyle-Shanks deleted the kj-Add-validation-to-single-track-upload-flow branch August 8, 2023 16:43
schottra added a commit that referenced this pull request Aug 8, 2023
* origin/main:
  Add nodes to env for SEO support (#3859)
  [C-2941] Modify cloudflare worker to pull in SEO data from discovery nodes (#3858)
  [C-2879] Add validation to single track upload flow (#3855)
  [C-2940] Update google analytics tags and fix embed build (#3856)
  [C-2852 PLAT-1094 PLAT-1093] Add fetch collection by permalink (#3751)
  v1.5.36
  Add DirectMessages Banner and Update All Banners (#3851)
  [PAY-1692] Rewrite 'Share to DMs' using less stateful logic (#3852)
  [C-2675][C-2692] Add multi track navigation sidebar and form controls (#3847)
  Fix send audio flow (#3850)
  Update SDK to latest 3.0.3-beta.109 (#3849)
audius-infra pushed a commit that referenced this pull request Aug 12, 2023
[3436c20] [PAY-1701] Fix "Share to DMs" to work through InboxUnavailableModal (#3874) Marcus Pasell
[a740243] Add sdk:update-hotfix (#3875) Dylan Jeffers
[a25fd19] [C-2759] Make donation link external (#3872) Dylan Jeffers
[15f056c] [PAY-1630] Wire up purchase content sagas (#3834) Randy Schott
[998d44b] Fix mobile crash on drawer dismiss (#3871) Reed
[7d0e0b3] [PAY-1430] "Leaving Audius" Warning for External Links (and createModal helper) (#3860) Marcus Pasell
[bee8bd1] Remove .only on upload cypress test (#3869) Raymond Jacobson
[4c0b25f] [C-2926] Implement selected values for upload contextual menu fields (#3848) Dylan Jeffers
[5773578] Preserve CIDs for track and collection cover arts (#3866) Marcus Pasell
[be0d278] [C-2930] Fix extra space after username in tip to unlock modal (#3845) nicoback2
[f5320be] QA-588 Fix collection card profile link  (#3853) nicoback2
[360416e] Fix broken playlist fetch via resolve (#3863) Raymond Jacobson
[2dc2c29] [PAY-1695] DMs: Entrypoint Analytics (#3862) Marcus Pasell
[f80d366] Minor improvements to SEO flow merged in #3859 (#3861) Raymond Jacobson
[b99d62f] Add nodes to env for SEO support (#3859) Raymond Jacobson
[20476ee] [C-2941] Modify cloudflare worker to pull in SEO data from discovery nodes (#3858) Raymond Jacobson
[7f79830] [C-2879] Add validation to single track upload flow (#3855) Kyle Shanks
[6f4fc89] [C-2940] Update google analytics tags and fix embed build (#3856) Raymond Jacobson
[3469c89] [C-2852 PLAT-1094 PLAT-1093] Add fetch collection by permalink (#3751) Dylan Jeffers
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants