-
Notifications
You must be signed in to change notification settings - Fork 1
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
Assessment import for multiple languages and identical slug #408
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -264,23 +264,31 @@ def add_field_values_to_assessment(self, assessment: Assessment) -> None: | |
assessment.high_result_page_id = ( | ||
None | ||
if self.high_result_page == "" | ||
else get_content_page_id_from_slug(self.high_result_page) | ||
else get_content_page_id_from_slug( | ||
self.high_result_page, self.locale.language_code | ||
) | ||
) | ||
assessment.medium_inflection = self.medium_inflection | ||
assessment.medium_result_page_id = ( | ||
None | ||
if self.medium_result_page == "" | ||
else get_content_page_id_from_slug(self.medium_result_page) | ||
else get_content_page_id_from_slug( | ||
self.medium_result_page, self.locale.language_code | ||
) | ||
) | ||
assessment.low_result_page_id = ( | ||
None | ||
if self.low_result_page == "" | ||
else get_content_page_id_from_slug(self.low_result_page) | ||
else get_content_page_id_from_slug( | ||
self.low_result_page, self.locale.language_code | ||
) | ||
) | ||
assessment.skip_high_result_page_id = ( | ||
None | ||
if self.skip_high_result_page == "" | ||
else get_content_page_id_from_slug(self.skip_high_result_page) | ||
else get_content_page_id_from_slug( | ||
self.skip_high_result_page, self.locale.language_code | ||
) | ||
) | ||
assessment.skip_threshold = self.skip_threshold | ||
assessment.generic_error = self.generic_error | ||
|
@@ -398,15 +406,19 @@ def from_flat(cls, row: dict[str, str], row_num: int) -> "AssessmentRow": | |
) | ||
|
||
|
||
def get_content_page_id_from_slug(slug: str) -> int: | ||
def get_content_page_id_from_slug(slug: str, locale: str) -> int: | ||
try: | ||
page = ContentPage.objects.get(slug=slug) | ||
page = ContentPage.objects.get(slug=slug, locale__language_code=locale) | ||
except ObjectDoesNotExist: | ||
raise ImportAssessmentException( | ||
f"You are trying to add an assessment, where one of the result pages " | ||
f"references the content page with slug {slug} which does not exist. " | ||
f"references the content page with slug {slug} and locale {locale} which does not exist. " | ||
"Please create the content page first." | ||
) | ||
except ContentPage.MultipleObjectsReturned: | ||
raise ImportAssessmentException( | ||
f"Multiple ContentPages found with slug '{slug}' and locale '{locale}'. Please ensure slugs are unique within each locale." | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no test for this, but also it should be possible to have the same slug for two pages in the same locale, so I don't think we can create a test for this? If we can't create a test because it's not possible to get in this situation, then we can probably remove this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can't create a test for this with this new fix so I'll remove it. |
||
return page.id | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
title,question_type,tags,slug,version,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,skip_high_result_page,generic_error,question,explainer,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id,answer_responses | ||
Test min max range,integer_question,test-min-max-range,test-min-max-range,v1.0,en,,5.0,,3.0,,0.0,,This is a generic error,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, | ||
Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5.0,medium-score,1.0,low-score,0.0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, |
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.
Instead of sending the language code here, and then doing a join in the db, why not just send the actual locale object to this function? i.e.
locale: Locale
Then calling the function becomes
get_content_page_id_from_slug(self.low_result_page, self.locale)
, and the filter becomesContentPage.objects.get(slug=slug, locale=locale)
, simplifying it.