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

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

Open
fayyazahmed66 opened this issue Aug 11, 2023 · 14 comments
Labels
quince release testing Affects the upcoming release (attention needed)
Milestone

Comments

@fayyazahmed66
Copy link

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
Copy link
Member

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
Copy link

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
Copy link

sambapete commented Sep 5, 2023

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

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
Copy link

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
Copy link

sambapete commented Sep 5, 2023

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
Copy link

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
Copy link

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
Copy link

sambapete commented Sep 6, 2023

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
Copy link

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

regisb commented Sep 19, 2023

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='[email protected]').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
Copy link
Contributor

regisb commented Sep 19, 2023

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.

regisb added a commit to overhangio/tutor-ecommerce that referenced this issue Sep 19, 2023
These changes were implemented to research the following issue:
openedx/wg-build-test-release#301
@sambapete
Copy link

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.

Faraz32123 pushed a commit to edly-io/ecommerce that referenced this issue Sep 25, 2023
fix course detail page url as after creating a course, its detail page was not accessible and showing nothing.
The course page was unavailable when the course run includes a dot, as a result it's url was mismatching and it is fixed by adding the trailing slash "/" to the url.

Here's the link to the initial GitHub issue: openedx/wg-build-test-release#301
Faraz32123 pushed a commit to overhangio/tutor-ecommerce that referenced this issue Oct 10, 2023
These changes were implemented to research the following issue:
openedx/wg-build-test-release#301
Faraz32123 pushed a commit to edly-io/ecommerce that referenced this issue Nov 21, 2023
fix course detail page url as after creating a course, its detail page was not accessible and showing nothing.
The course page was unavailable when the course run includes a dot, as a result it's url was mismatching and it is fixed by adding the trailing slash "/" to the url.

Here's the link to the initial GitHub issue: openedx/wg-build-test-release#301
@mariajgrimaldi mariajgrimaldi added this to the Quince.3 milestone Feb 12, 2024
Faraz32123 pushed a commit to edly-io/ecommerce that referenced this issue Mar 21, 2024
fix course detail page url as after creating a course, its detail page was not accessible and showing nothing.
The course page was unavailable when the course run includes a dot, as a result it's url was mismatching and it is fixed by adding the trailing slash "/" to the url.

Here's the link to the initial GitHub issue: openedx/wg-build-test-release#301
@mariajgrimaldi mariajgrimaldi modified the milestones: Quince.3, Redwood.1 Apr 26, 2024
@mariajgrimaldi mariajgrimaldi added the release testing Affects the upcoming release (attention needed) label May 9, 2024
@mariajgrimaldi mariajgrimaldi modified the milestones: Redwood.1, Redwood.3 Sep 14, 2024
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Oct 29, 2024

I left a comment on the PR, but if we don't get this merged I'll close it, considering that e-commerce will be archived after Sumac.

@mariajgrimaldi mariajgrimaldi modified the milestones: Redwood.3, Sumac.1 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quince release testing Affects the upcoming release (attention needed)
Projects
Status: Backlog
Development

No branches or pull requests

4 participants