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

rename url slug(not allowed) of static tabs on course import #3152

Closed
wants to merge 1 commit into from

Conversation

zubair-arbi
Copy link
Contributor

@cahrens
Copy link

cahrens commented Apr 1, 2014

@nasthagiri You might want to review this PR.

@nasthagiri
Copy link
Contributor

@zubair-arbi Hi Zubair, what is the root reason for why there could be static tabs with url_slug values of "static"?

@zubair-arbi
Copy link
Contributor Author

@nasthagiri users can add/modify static tabs through editing exported xml course and re-import it. In this way they can change 'url_slug' to some verbose string e.g. 'static', instead of some hash string (through studio).

@nasthagiri
Copy link
Contributor

@zubair-arbi So why just replace url_slugs that have the value "static"? Why don't we generate a new random url_slug for all static tabs when they are imported?

@zubair-arbi
Copy link
Contributor Author

The thing is nginx configuration checks for '/static/' in url and checks for staticfiles against it. So I guess there is no need for us to change all url_slugs on import.

@nasthagiri
Copy link
Contributor

@zubair-arbi @adampalay I have done some digging into this issue.

The problem is that the original course, https://edge.edx.org/courses/edX/X801/2013_Fall/, has a tab, "Peer Groups", with an invalid URL. If you click on "Peer Group", it will take you to a 404 Not Found error.

Then, when Piotr exported that course and imported it, it continued to have the invalid static tab location (url_slug = "static").

So this issue is unrelated to the nginx configuration. You would get the same behavior if an invalid configured course had any other invalid url_slug such as "something_completely_different".

So here are possible paths to proceed:

  1. Figure out how the original course, https://edge.edx.org/courses/edX/X801/2013_Fall/, got in the invalid state in the first place.
  2. Update docs to state not to mess with the "url_slug" attribute of the course. All static tabs must have url_slugs. And all url_slugs must have its namesake corresponding file in the "tabs" folder of the XML tar.gz file.
  3. Change the XML import code to validate url_slugs of static tabs by making sure the location of the url_slug is valid.

@zubair-arbi
Copy link
Contributor Author

@nasthagiri Please check my comment on relative ticket STUD-1407.

@nasthagiri
Copy link
Contributor

@zubair-arbi Thanks for clarifying the issue.
Given that Piotr's escalation has been resolved by Chris already, I'd like to see if we can take the time to address this more holistically in devops and in xmodule.

Ideally, we should tackle it from the following two fronts:

  1. @feanil Can we relax the nginx rule so it allows pages such as https://courses.stage.edx.org/courses/zub/footest1/2014_ft1/static/ ?
  2. In xmodule, the XML import code should validate all static tabs to make sure the corresponding page exists and is accessible. If not, it should delete that tab and mark it as an error. This way, there would not be a different behavior on both LMS and CMS.

@feanil
Copy link
Contributor

feanil commented Apr 3, 2014

@nasthagiri @zubair-arbi the nginx rules are all for urls where the root is '/static' not for all urls that have static in the name.

@nasthagiri
Copy link
Contributor

@feanil I agree that the intent of the nginx rules must be to match the '/static' string in the beginning of the URL. However, checkout the following proof-of-concept test course I created with "static" as the "course number":

https://studio.edge.edx.org/tabs/edx.static.2014/branch/draft/block/2014
If you click on "View Live", it takes you to the following page:
https://edge.edx.org/courses/edx/static/2014/jump_to/i4x://edx/static/course/2014
which displays a 404 Not Found nginx error.

@feanil
Copy link
Contributor

feanil commented Apr 3, 2014

@nasthagiri I see, I've created a ticket on the devops board to investigate this further. I think it makes sense to only have the static rule applied at the root of the url and not for just anywhere in the url.

@nasthagiri
Copy link
Contributor

@feanil Thanks!

@cahrens Should there be a separate ticket for #3?
3. Change the XML import code to validate url_slugs of static tabs by making sure the location of the url_slug is valid.
If so, should it be assigned to the studio team?

@nasthagiri
Copy link
Contributor

Here's the new devops ticket for updating the nginx rule:
https://edx-wiki.atlassian.net/browse/TKTS-1464

@zubair-arbi
Copy link
Contributor Author

Closing this PR in favor of STUD-1515

@zubair-arbi zubair-arbi closed this Apr 7, 2014
PYushchenko added a commit to smartdec/edx-platform that referenced this pull request Apr 25, 2014
@benpatterson benpatterson deleted the zub/bugfix/std1407-statictabs branch January 21, 2015 13:11
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.

4 participants