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

Assessment import for multiple languages and identical slug #408

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

DevChima
Copy link
Contributor

@DevChima DevChima commented Feb 5, 2025

Purpose

When looking up content pages in the assessments import, we're only doing so from the slug, not the locale. So if there's a page with a translation, we'll find multiple results and the import will break.

Solution

Look up content pages with both slug and locale

Checklist

  • Added or updated unit tests
  • Added to release notes
  • Updated readme/documentation (if necessary)

except ContentPage.MultipleObjectsReturned:
raise ImportAssessmentException(
f"Multiple ContentPages found with slug '{slug}' and locale '{locale}'. Please ensure slugs are unique within each locale."
)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@@ -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:
Copy link
Contributor

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 becomes ContentPage.objects.get(slug=slug, locale=locale), simplifying it.

@rudigiesler
Copy link
Contributor

Also, could you update the release notes?

@DevChima DevChima merged commit 5512621 into main Feb 5, 2025
2 checks passed
@DevChima DevChima deleted the assessment-multiple-language-import-fix branch February 5, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants