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

feat: move mako logic to init method #98

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Conversation

andrey-canon
Copy link
Collaborator

Description

This covers the edxmako implementation that was proposed in this pr #72 and add some extra changes like removing old implementation and adding unit testing and basically this allows to use any eox-nelp template outside a specific view.

This doesn't include the payment notification template

Testing instructions

  1. Go to /eox-nelp/frontend/experience/feedback/courses/
  2. Go to /eox-nelp/stats/tenant/
  3. Go to the home page, this implementation requires the last version of the nelp bragi theme

Copy link
Collaborator

@johanseto johanseto left a comment

Choose a reason for hiding this comment

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

I didnt understood why I need the version of bragi theme.
I think this is only an eox-nelp change with its views, by the way, the refactor is working for my local env.
Peek 2023-09-18 12-14

@@ -47,4 +47,4 @@ def get_tenant_stats(request):
context = {query_param: "true" for query_param in STATS_QUERY_PARAMS}
context.update(request.GET.dict())

return render_to_response("tenant_stats.html", context)
return edxmako.shortcuts.render_to_response("tenant_stats.html", context, "main", request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is need the arg "main" to this render_to_response method,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I think this is related to that main dirs or something to do it with that way.
https://github.com/eduNEXT/eox-nelp/pull/98/files#diff-ec9dfd84694298d271197a0909b7b62e0301e9df3dce5c29d0cec0a8255ebd42L19

if path_to_templates not in edxmako.LOOKUP['main'].directories:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works without those extra parameters(main and request), however when I was testing that raised an missing parameters error so I decided to pass the default value for the namespace as we do in the current implementation and the current request

run_init_pipeline()

set_mako_templates_mock.assert_called_once()
patch_user_gender_choices_mock.assert_called_once()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok this add gender choices called test xD

@andrey-canon
Copy link
Collaborator Author

andrey-canon commented Sep 18, 2023

I didnt understood why I need the version of bragi theme. I think this is only an eox-nelp change with its views, by the way, the refactor is working for my local env.

There are two parts where the stats and feedback views are called, their own page and the home page so I mentioned the last bragi version since this change just works with the version of eox-nelp that includes the payment notification version

@andrey-canon andrey-canon merged commit e145f56 into master Sep 20, 2023
8 checks passed
Ever3tt pushed a commit that referenced this pull request Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants