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

Encapsulate valid label fetching to FlaggableElement #593

Merged
merged 8 commits into from
Jan 11, 2023

Conversation

fozziethebeat
Copy link
Collaborator

@fozziethebeat fozziethebeat commented Jan 10, 2023

Closes #584

This creates a new api route /api/valid_labels that returns the list of valid labels that can apply to any message.

FlaggableElement is updated to dynamically fetch labels from the new route and cache them. This allows FlaggableElement to be used anywhere without needing to pipe in the valid labels from call sites. This still calls the api route once per page load.

… from that within FlaggableElement. This allows FlaggableElement to fetch all its own data and remove the need to pipe labels through a series of components
@fozziethebeat
Copy link
Collaborator Author

Blocking on #590 before marking ready for review so I can merge.

@othrayte
Copy link
Collaborator

I see code 422 (unprocessable entity) responses when submitting the label report on the rank pages, is this expected? Also I get the same on the labeling pages (but I'm not sure is we are planning to have the flags there).

@fozziethebeat
Copy link
Collaborator Author

Good catch, I didn't catch that. Fixed now

@fozziethebeat
Copy link
Collaborator Author

Posted #614 as a future cleanup around messages. The types are pretty confusing and different depending on use case.

import { oasstApiClient } from "src/lib/oasst_api_client";

/**
* TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo what?

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 see you did the review carefully. TODO was to write the handler comments. done :)

/**
* TODO
*/
const handler = async (req, res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use one of the middlewares here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@AbdBarho AbdBarho merged commit bdb4762 into main Jan 11, 2023
@AbdBarho AbdBarho deleted the 584-valid-labels-everywhere branch January 11, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Cleanup: Ensure FlaggableElement always has a list of valid labels
3 participants