-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add content tag models #215
Conversation
…ser_id hardcoded into QA and WhatsApp endpoints - to be inferred from bearer token later.
… of a "question" parameter
…ators' expectation
…ss to auth information
…nd update Dockerfile deployment
* Add key management endpoint pytest * TEMP: Add test for user functions (not working due to async issue) * fix scope of asession in test_urgency_detect (future-proofing) * Fix user tests with async session * Add where to set/find login credentials to docs * Temporary fix for rotation endpoint test * Fix scope mismatch (some UD tests break) * Add multi-user tests for content CRUD * Add multi-user tests for UD CRUD + add fetch check first in delete endpoint * Add user2 token denial test to retrieval and UD detect * Put multi-user tests under a different class * fixed async tests in session scope issue * Make test user_ids integers to match new schema * Fix handling of user_ids in pytest * Separate content CRUD multi-user tests and use fixture * Separate UD CRUD multi-user tests and use fixture --------- Co-authored-by: Sid Ravinutala <sid.ravi1@gmail.com>
* Add key management endpoint pytest * TEMP: Add test for user functions (not working due to async issue) * fix scope of asession in test_urgency_detect (future-proofing) * Fix user tests with async session * Add where to set/find login credentials to docs * Temporary fix for rotation endpoint test * Fix scope mismatch (some UD tests break) * Add multi-user tests for content CRUD * Add multi-user tests for UD CRUD + add fetch check first in delete endpoint * Add user2 token denial test to retrieval and UD detect * Put multi-user tests under a different class * fixed async tests in session scope issue * Make test user_ids integers to match new schema * Fix handling of user_ids in pytest * Separate content CRUD multi-user tests and use fixture * Separate UD CRUD multi-user tests and use fixture * Playground: Add type of search to user message display * Make dashboard functions filter on user_id * Playground: change "JSON" to "<json>" in error message response. * UD page: Make background white for now * move key_management tests to run last :) * Remove incorrect retrieval tests * improve query type formatting and use save button for ud rule --------- Co-authored-by: Sid Ravinutala <sid.ravi1@gmail.com> Co-authored-by: Suzin You <7042047+suzinyou@users.noreply.github.com>
💰 Infracost reportMonthly cost will decrease by $0.40 📉
*Usage costs can be estimated by updating Infracost Cloud settings, see docs for other options. Cost details (includes details of unsupported resources)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, thanks for lots of great thoughtful code! Left a few comments that should be easy to address. Will quickly approve once you've considered them! Mainly preventing the user from adding/editing tags to have the same name
<Layout.Spacer multiplier={2} /> | ||
<Autocomplete | ||
multiple | ||
limitTags={3} | ||
id="tags-autocomplete" | ||
options={availableTags} | ||
getOptionLabel={(option) => option!.tag_name} | ||
value={contentTags} | ||
onChange={handleTagsChange} | ||
renderInput={(params) => ( | ||
<TextField | ||
{...params} | ||
variant="outlined" | ||
label="Tags" | ||
placeholder="Add Tags" | ||
/> | ||
)} | ||
sx={{ width: "500px" }} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to above the content box as per the figma design? So put it inside AddEditContentPage
below the Header component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am open to put the tags on top of the Language Bar, but I will keep it within the ContentBox, to make it easier to update content_tags within contents
core_backend/app/contents/routers.py
Outdated
user_db.user_id, content.content_tags, asession | ||
) | ||
if not is_tag_valid: | ||
raise HTTPException(status_code=400, detail=f"Invalid tags id: {content_tags}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/nit does this work for many tags at once? then we should say "Invalid tag ids: " instead of tags id. Same with the next one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, thanks for the changes!
Only thing I just noticed - the content tags table is called content_tags
whereas the new standard we're following would be content-tags
. Could you change this before merging? Thanks :)
Ideally all table names would be singular (so content-tag) but I can see how that could get confusing in the code so happy to keep it as tags if that makes more sense.
Reviewer: @amiraliemami
Estimate: 1h
Ticket
Fixes: AAQ-523
Description
Goal
The goal is to add implement adding tags to contents
Changes
On backend:
Future Tasks (optional)
How has this been tested?
Tests have been implemented and run. Frontend has been tested as well.
Checklist
Fill with
x
for completed. Delete any lines that are not relevant