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 metadata types #1013

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Fix metadata types #1013

merged 1 commit into from
Feb 15, 2024

Conversation

fengelniederhammer
Copy link
Contributor

PR Checklist

  • [ ] All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

Copy link
Contributor

@TobiasKampmann TobiasKampmann left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

How did this manifest pre-PR? Was anything broken? What's possible now that wasn't before?

@fengelniederhammer
Copy link
Contributor Author

Ah yes, I wanted to add a screenshot, but forgot. This happens on the review page:
grafik
(The component probably crashes, thus no metadata is shown)

On the edit page, there is a validation error from zod that the types don't match:
grafik
I.e. we need to take care of null values there.

@fengelniederhammer
Copy link
Contributor Author

We should probably rework this at some point to show less frightening errors to the users:

  • Either we know the error is fine and show it in a nice box
  • or we show something like "Oops, something went wrong, please contact the admin with this log trace id"

@corneliusroemer
Copy link
Contributor

Thanks so much @fengelniederhammer - I had no clue what this PR was fixing and your summary makes it very easy to understand!

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Feb 13, 2024

Do you want to test in a preview? 😜 I didn't realize this isn't actually merging into main, so ignore - it can be tested in the main PR

@corneliusroemer corneliusroemer added the preview Triggers a deployment to argocd label Feb 13, 2024
@fengelniederhammer
Copy link
Contributor Author

I found it while reviewing the PR of the base branch of this - so I didn't want to clutter that. I'll wait until that is merged and merge this to main then.

@TobiasKampmann TobiasKampmann force-pushed the 940-query-metadata-for-reviewcards branch 4 times, most recently from 5646075 to e725672 Compare February 14, 2024 12:11
Base automatically changed from 940-query-metadata-for-reviewcards to main February 14, 2024 12:39
@fengelniederhammer fengelniederhammer force-pushed the fixMetadataTypes branch 2 times, most recently from c4ce8e3 to 76e4a9a Compare February 15, 2024 12:47
unprocessed metadata is always Map<String, String>, processed metadata fields can be null
@fengelniederhammer fengelniederhammer merged commit 061de49 into main Feb 15, 2024
11 of 12 checks passed
@fengelniederhammer fengelniederhammer deleted the fixMetadataTypes branch February 15, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants