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.

Sign up for GitHub

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

Jump to bottom

Use internal IA for the AAQ instead of a config. #6160

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Use internal IA for the AAQ instead of a config. #6160

merged 1 commit into from
Aug 8, 2024

Conversation

akatsoulas
Copy link
Collaborator

No description provided.

@akatsoulas akatsoulas requested review from escattone and removed request for escattone August 2, 2024 13:32
@akatsoulas akatsoulas marked this pull request as draft August 2, 2024 13:33
@akatsoulas akatsoulas mentioned this pull request Aug 2, 2024
Closed
@akatsoulas akatsoulas force-pushed the aaq-topics branch 13 times, most recently from 6020e03 to 001d35b Compare August 7, 2024 12:11
@akatsoulas akatsoulas marked this pull request as ready for review August 7, 2024 12:11
@akatsoulas akatsoulas requested a review from escattone August 7, 2024 12:11
escattone
escattone requested changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

Overall, this is really nicely done and a really nice step forward, thanks @akatsoulas!
kitsune/dashboards/readouts.py Show resolved Hide resolved
kitsune/products/views.py Outdated Show resolved Hide resolved
kitsune/questions/admin.py Show resolved Hide resolved
kitsune/questions/forms.py Show resolved Hide resolved
kitsune/questions/jinja2/questions/includes/aaq_macros.html Outdated Show resolved Hide resolved
kitsune/questions/jinja2/questions/includes/aaq_macros.html Outdated Show resolved Hide resolved
kitsune/questions/jinja2/questions/question_list.html Outdated Show resolved Hide resolved
kitsune/wiki/views.py
@@ -90,7 +90,7 @@ def _doc_page_cache_view(request, document_slug, *args, **kwargs):
if (
request.user.is_authenticated
or request.GET.get("redirect") == "no"
or request.session.get("product_key")
or request.session.get("product_slug")
Copy link
Contributor

@escattone escattone Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I tried to find where we stuffed the product_slug or product_key into the session object, and I couldn't find anyplace where we do that. Maybe we don't need this line any longer?
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 updated the key to product_slug in the aaq view.
https://github.com/mozilla/kitsune/blob/main/kitsune/questions/views.py#L535
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch!
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we ever insert the product_slug key into the top level of the request.session, only within the dictionary of the aaq_context key, right? If so, it seems that we could remove this? It's not important and unrelated. I was just curious.
kitsune/questions/views.py Show resolved Hide resolved
@akatsoulas akatsoulas force-pushed the aaq-topics branch 3 times, most recently from a93d9b6 to 4ec1ebe Compare August 8, 2024 14:13
escattone
escattone requested changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@escattone escattone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

With the change from aaq_context.key to aaq_context.product_slug, I think there are still some instances of aaq_context.key that need to change to aaq_context.product_slug.
kitsune/products/views.py Outdated Show resolved Hide resolved
kitsune/questions/jinja2/questions/includes/aaq_macros.html Outdated Show resolved Hide resolved
kitsune/users/jinja2/users/auth.html Outdated Show resolved Hide resolved
kitsune/wiki/views.py
@@ -90,7 +90,7 @@ def _doc_page_cache_view(request, document_slug, *args, **kwargs):
if (
request.user.is_authenticated
or request.GET.get("redirect") == "no"
or request.session.get("product_key")
or request.session.get("product_slug")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we ever insert the product_slug key into the top level of the request.session, only within the dictionary of the aaq_context key, right? If so, it seems that we could remove this? It's not important and unrelated. I was just curious.
@escattone escattone merged commit 7b2b199 into mozilla:main Aug 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants
@akatsoulas @escattone