Unified Course Team and Library Users page. Converted both to Backbone.

[e-kolpakov]

Background: Library Users page was created by copy-pasting of Course Team due to Course Library MVP deadline. This PR contains a unification of two pages and switching to using Backbone and underscore
Jira tickets: SOL-194, OC-45
Discussions: Previous PR, SOL-194
Sandbox: LMS, Studio
Dependencies: none
Test instructions: Library Users and Course Team pages should work as expected, namely:

1. Admin can add/delete, promote/demote users.
2. Non-admin users cannot add/delete, promote/demote other users (changed behavior: Course Users page did not allow deleting current user from course)
3. Users can delete themselves from course/library.
4. There must always be at least one admin, hence last admin cannot demote or delete himself (changed behavior: Course Team did not allow deleting current user; Library Users did not allow deleting current user if (s)he was last admin)
5. Course Team use two roles: 'staff' and 'instructor' (aka "Admin"); Library Users use three: 'library_user' (aka "User"), 'staff' and 'instructor' (aka "Admin").

Author concerns: new bok_choy tests might be flaky - one out of nine tests failed at setUp due to 404 in studio. Refreshing page object after every modification seems fixed the issue as page is refreshed each time, but the reason why only one random test failed out of nine is unclear.

[openedx-webhooks]

Thanks for the pull request, @e-kolpakov! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?number=7336&repo=edx%2Fedx-platform

[e-kolpakov]

@cahrens SOL-194 says that second reviewer would be on T&L team, so could you please assign someone to make a review?

[cahrens]

@e-kolpakov Why does this PR contain translation changes?

Please provide a link to the first review. I don't know if we will be able to review this PR in our current sprint, but I'll see what we can do.

[e-kolpakov]

@cahrens First review will happen here.

There are some translation changes: some strings were moved from html templates to underscore, so they moved from django messages to djangojs. If those files are not supposed to be included into PRs I'll remove them, but it looks like the mechanism that prevented them to be committed is a bit broken.

Also, is this "test this please" thing work? I was unable to get a build fired using this, but haven't heard anything about test team changing (or disabling) it.

There's no hurry with this, so take your time with reviewing. Thank you!

[cahrens]

Please check with @sarina about how she wants to handle this translation update.

And if "test this please" isn't working for you, please check with testeng (@benpatterson). I don't know if the functionality is broken, or if you aren't on a list that allows is to work (being an external contributor). It has worked for me, but I haven't tried it recently.

[sarina]

@e-kolpakov generally changes to .po/.mo files aren't checked in by developers. It causes a larger diff and means there's a burden to keep updating the files as you change them. I merge in updates to these files once per week, on Monday mornings, so you don't have to worry about them.

[sarina]

Where do you define this string? Please use NAMED variables, not "%s". See http://edx.readthedocs.org/projects/edx-developer-guide/en/latest/internationalization/i18n.html#general-internationalization-rules

[e-kolpakov]

@sarina I don't. It's from other commit: edx@e21ff20#diff-1dcea18a471b3b764091199f6d021480R2555

[e-kolpakov]

@sarina ok, thanks for the explanation. BUt should I remove them from PR or leave as is?

[sarina]

@e-kolpakov I would recommend removing them from your PR - if you generate them locally, it should be for testing purposes & you shouldn't check them in.

[e-kolpakov]

@sarina removed.

[benpatterson]

@e-kolpakov the test comment will only work if you are whitelisted.

I'll add you to the whitelist now (only an admin can do this, which includes @sarina): add to whitelist

[e-kolpakov]

@benpatterson understood, thank you.

[Kelketek]

Nit: Newline missing here.

[Kelketek]

@e-kolpakov The test failure is a little worrisome. I would expect to get a different error than a timeout if it were a 404 in this instance. Do you have some reason to believe a 404 was given? bok_choy's visit() function should only see if that URL loads at all, not that the page that's loaded is particularly ready. To me, this seems to indicate that the server didn't respond fast enough. But since the timeout is 60 seconds, I'd think it would have enough time to render the course team page.

It's possible Jenkins was doing some DNS lookup and got stuck while doing so or something, preventing the server from loading. But I'd like @jzoldak to weigh in. It seems odd that this test failure in particular is what hit us if that's the case, since the test is within the scope of the PR, and there are many others it could have happened to, statistically. Jay, is there a better explanation? Do I understand visit() correctly?

[Kelketek]

@e-kolpakov Please don't forget to set up a sandbox for this. The team will want to verify things behave as expected when they click around.

[e-kolpakov]

@Kelketek @jzoldak I've investigated this thing a bit. Turns out, this is a very weird bug, and it's reasons still unknown to me. My observations:

• Any test out of those nine added can fail.
• Fail happens in setUp function
• It is always caused by 404 error.
• Despite 500 there're no error records in bok_choy studio logs
• Failed page URL is: http://localhost:8031/course_team/course-v1:test_org+2B336790434684799951055756418812500001273+test_run
• If I manually reload the page it still shows 404
• If I manually go to dashboard the course with selected number exists.
• When I manually go to course team page for that course URL is exactly the same as before, but page opens and test continues to success.
• If there are multiple page reloads within the test (e.g. test_admins_should_be_able_to_remove_other_admins) after reloading it throws 500 again. Same behavior as before: if manually navigated to course team page test continues to success.
• Tests that pass with no user interactions access course team page at http://localhost:8031/course_team/course-v1%3Atest_org%2B336790434684799951055756418812500001273%2Btest_run, i.e. the same url with course id urlencoded.

I'm investigating it further. The only reason I can think of is that "broken" URL is non-UTF8 encoded, so something breaks later.

Meanwhile, I've addressed @Kelketek 's notes and will push an update in a minute.

[e-kolpakov]

@Kelketek @jzoldak Update: urls are not quite the same; broken URL does not contain a trailing slash. Which means that for some reason URL normalizing (i.e. appending trailing slash if it's not present) does not work from time to time.

[Kelketek]

@e-kolpakov Check the URL patterns. If Django does not detect that the URL pattern matches, it may (depending on configuration) append a slash internally before trying the route again, and finally giving a 404.

[e-kolpakov]

@Kelketek actually it's quite the opposite: url with trailing slash works, without does not work. Since there's a APPEND_SLASH directive and it's set to True (implicitly: default is true and it's not overridden anywhere). It should append slash and try again. But for some reason it does not work from time to time.

URL pattern is the following: r'^course_team/{}/(?P<email>.+)?$'.format(COURSELIKE_KEY_PATTERN), so trailing slash is required here. The only other pattern in the cms/urls.py that requires trailing slash is r'^home/$'.

This explains why there weren't any acceptance test issues before: this url was not tested using bok choy; and page url constructed differently in lettuce (always with trailing slash). So, since the common approach is to allow missing trailing slash, I'll update url pattern to be permissive as well.

[Kelketek]

@e-kolpakov That's very strange. I wonder if the 404 was generated by it not finding a matching pattern, or by it matching something different than we expected, and raising a 404 when that function did not find what it expected. The latter might explain intermittent behavior if it's possible for URLs to be checked in a different order for some reason. I can't think of any cause for this off the top of my head, however, and your solution sounds like it may be the most practical anyway.

[e-kolpakov]

@Kelketek url starts with course_team - it's unlikely to match anything different. But I'll take a look :)

[cahrens]

Update comment-- this is for both course teams and library teams.

[cahrens]

Done reviewing-- nice work! The only thing I'm squeamish about his the fact that part of the page (in particular, the add user form) is still being rendered server-side, increasing the amount of "reaching out" that the Backbone view has to do for handling events. We have commonly left that special "new button" on the RHS out of the Backbone code (which does lead to a bit of ugliness), but I'd love to see the new user form itself moved to Backbone.

[e-kolpakov]

@cahrens I've addressed minor notes, but I would like to discuss "convert new user form to Backbone". Do you think this PR is good enough with new user form still rendering server-side? I would like to do this conversion in smaller chunks if possible. I mean, converting the rest of the page to backbone would be a separate PR later (likely with dedicated task to allow for scheduling/prioritizing).

Also, I have no better commit message for last commit than "Addressed review notes", so I promise I'll squash commits when we're done with review.

[cahrens]

Remove comment.

[cahrens]

👍 after two minor issues noted above are fixed. Thanks for the good work!

@e-kolpakov I agree with handling the new user form as a separate task.