openedx / wg-build-test-release

Open edX Build / Test / Release Working Group
24 stars 14 forks source link

[ECOM > Create a course seat] > After creating course I am not able to open its detail page #301

Open fayyazahmed66 opened 11 months ago

fayyazahmed66 commented 11 months ago

I was testing the course seat creation on Ecom and I noticed that when I create a course on Studio having course run like 2000.01, and then create a course seat on Ecom. It will create course seats successfully. But when I try to open that course seat to see the details, it doesn't show anything. The problem is with the course run has zero after . operator. i-e 2000.01. We need to fix the regex I think. Steps to reproduce:

  1. Create a course on studio and set its course run to like 2000.01
  2. Publish the course
  3. Go to Ecom and create a course seat
  4. check the course seat detail page after creating a seat

Expected result: It should show course seat detail.

Actual Result: Not found is showing in the network tab.

Note: I can reproduce this issue if we installed the ECOM service on our Demo instance.

Screenshot at Aug 08 15-07-35 Screenshot at Aug 08 15-20-11
mariajgrimaldi commented 10 months ago

I don't have access to an e-commerce installation, so I can't test this to confirm the issue. @sambapete can you lend us a hand here? Thanks!

Also, if you think you know how to solve this @fayyazahmed66, open a PR! We would appreciate it. Not a lot of us have access to e-commerce installations.

sambapete commented 10 months ago

Thanks for the reminder @mariajgrimaldi. I had planned to look into it when @fayyazahmed66 raised the issue and try it on one of our systems but completely forgot about it after a few days.

Right now, I can test it on a nutmeg.3 instance (a copy of our production instance) and a palm.2 instance (where we are testing our own fork).

I will first try to reproduce the problem.

sambapete commented 10 months ago

@fayyazahmed66 I would need more details about what you were trying because I can definitely access the product details for the course I created.

Question: are you using the Course Administration Tool to create your courses in ecommerce?

For example, in my case it would be https://ecommerce.cours-virginie.edulib.org/courses/

Here are some screenshots from a course I just created under palm.2

Capture d’écran, le 2023-09-05 à 10 35 34 Capture d’écran, le 2023-09-05 à 10 36 44 Capture d’écran, le 2023-09-05 à 10 37 09
sambapete commented 10 months ago

I see what you mean now. It is when trying to access the course in the course administration tool not in the products page for Oscar.

Capture d’écran, le 2023-09-05 à 10 44 03
sambapete commented 10 months ago

I also tried a course with a session ending with .9 instead of .09 and I had the same behavior.

course-v1:UMontreal+Session101.2+2023.9 and course-v1:UMontreal+Session101+2023.09

sambapete commented 10 months ago

I don't know how to solve it yet, but it is clear the problem would be in the ecommerce/ecommerce/core/constants.py file where COURSE_ID_REGEX is defined.

it is currently COURSE_ID_REGEX = r'[^/+]+(/|+)[^/+]+(/|+)[^/]+'

I tried using it with https://pythex.org/ and it does match course-v1:UMontreal+Session101.2+2023.9 and course-v1:UMontreal+Session101+2023.09

sambapete commented 10 months ago

Based on the substrings I suspect that it should be COURSE_ID_REGEX = r'[^/+]+(/|+)[^/+]+(/|+)[^/+]' but I will try it first in my fork of ecommerce.

sambapete commented 10 months ago

This does result in a new problem when I access /courses in ecommerce

Capture d’écran, le 2023-09-05 à 16 05 41

It could potentially be a problem with the table not with the regex.

sambapete commented 10 months ago

After trying a few things, I discovered that the issue seems limited to courses where the sessions ends with a . (dot) followed with a number. I tried ending the session with a . (dot) by itself or followed by a letter and it worked.

One thing I noticed is that when a seat is opened in order to edit it, it first calls https://ecommerce.cours-virginie.edulib.org/api/v2/courses/course-v1%3AUMontreal%2BPATHPV.1%2BE2020?include_products=true and then it calls https://ecommerce.cours-virginie.edulib.org/api/v2/courses/course-v1%3AUMontreal%2BPATHPV.1%2BE2020/?include_products=true

When we encounter the problem, it never adds the / before the ?include_products=true argument

If you add the / it will display the JSON data for this course, but not pretty printed like we expect to see it for a course where we can edit the data.

I still haven't figured out why this is the case.

sambapete commented 10 months ago

I don't understand why it behaves differently when the course id ends with a . (dot) followed by a number. See the screenshots:

Capture d’écran, le 2023-09-06 à 19 14 27 Capture d’écran, le 2023-09-06 à 19 14 40
regisb commented 10 months ago

Reproducing the issue is easy with Tutor:

  1. Enable discovery and ecommerce plugins, run tutor local launch
  2. Create admin user in ecommerce: tutor local run ecommerce ./manage.py shell -c "from django.contrib.auth import get_user_model; get_user_model().objects.filter(email='YOUREMAIL@ACME.COM').update(is_staff=True, is_superuser=True)"
  3. In studio, create course with ID course-v1:overhangio+alpha+2023.09
  4. In http://ecommerce.local.overhang.io/courses/ create course with the same ID
  5. Open http://ecommerce.local.overhang.io/courses/course-v1:overhangio+alpha+2023.09/
regisb commented 10 months ago

The problem does not come from the COURSE_ID_REGEX. It comes from the Django REST framework.

To troubleshoot I opened a shell in the ecommerce container:

tutor dev run ecommerce ./manage.py shell

Import Django url resolver:

from django.urls import resolve

Then, compare:

>>> resolve("/api/v2/courses/course-v1:overhangio+alpha+2023/")
ResolverMatch(func=ecommerce.extensions.api.v2.views.courses.CourseViewSet, args=(), kwargs={'pk': 'course-v1:overhangio+alpha+2023'}, url_name=course-detail, app_names=['api', 'v2'], namespaces=['api', 'v2'], route=^api/v2/courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)/$)

with:

>>> resolve("/api/v2/courses/course-v1:overhangio+alpha+2023.09")
ResolverMatch(func=ecommerce.extensions.api.v2.views.courses.CourseViewSet, args=(), kwargs={'pk': 'course-v1+overhangio+alpha+2023', 'format': '09'}, url_name=course-detail, app_names=['api', 'v2'], namespaces=['api', 'v2'], route=^api/v2/courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)\.(?P<format>[a-z0-9]+)/?$)

Notice the 'format': '09' part. "09" is interpreted as a format suffix. Because "09" is an unknown format calling the url results in 404.

The problem can be diagnosed more precisely with:

>>> import ecommerce.extensions.api.v2.views.courses as course_views
>>> from rest_framework_extensions.routers import ExtendedSimpleRouter as SimpleRouter
>>> from rest_framework.urlpatterns import format_suffix_patterns
>>> router = SimpleRouter()
>>> router.register(r'courses', course_views.CourseViewSet, basename='course')
>>> format_suffix_patterns(router.urls)
[<URLPattern '^courses/$' [name='course-list']>, <URLPattern '^courses\.(?P<format>[a-z0-9]+)/?$' [name='course-list']>, <URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)/$' [name='course-detail']>, <URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)\.(?P<format>[a-z0-9]+)/?$' [name='course-detail']>, <URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)/publish/$' [name='course-publish']>, <URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)/publish\.(?P<format>[a-z0-9]+)/?$' [name='course-publish']>]

Notice the course-detail url pattern:

<URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)\.(?P<format>[a-z0-9]+)/?$' [name='course-detail']>

The "format" part from the regex matches ".09":

>>> re.match(r'^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)\.(?P<format>[a-z0-9]+)/?$', 'courses/course-v1+overhangio+alpha+2023.09').groupdict()
{'pk': 'course-v1+overhangio+alpha+2023', 'format': '09'}

This "non-formatted" detail url does not match because the url does not include a trailing slash: <URLPattern '^courses/(?P<pk>[^/+]+(/|\+)[^/+]+(/|\+)[^/]+)/$' [name='course-detail']>. Notice how the trailing slash is optional in the "formatted" url.

This seems to be a related issue: https://stackoverflow.com/questions/27963899/django-rest-framework-using-dot-in-url

We can resolve this issue by appending a trailing slash to the url. This url is created by backbone in "course_model.js". I managed to fix the issue with the following piece of code:


urlRoot: '/api/v2/courses/',
url: function() {
    return Backbone.Model.prototype.url.call(this) + '/';
},

When we define the above "url" method, the course page loads fine.

If this is a satisfactory fix then I can open a PR.

sambapete commented 10 months ago

Thanks for the detailed explanation @regisb I learned a few things reading it.

I agree that adding a trailing slash to the url does seem to solve the current issue and it's what I had noticed during my testing when I added a trailing slash to the url in order to see the JSON data.

I would be fine with this PR. Thanks again for your help.