Adding a courseware tab

[Chris Rossi]

Hi,

I'm working on the Personal Online Courses feature for MIT and today, on our branch, I added a 'POC Coach' tab.

The tab code is here:

https://github.com/jazkarta/edx-platform/commit/72f33668dfe60e82dcd52596b03348814210e6d7#diff-ef93f13ec297285f24d5598368de2064R714

An unresolved issue with this code is that the tab should really only be shown to users with the POC Coach role.  This gets difficult to implement, though, because the arguments to can_display just don't have the information we need.  It looks like we're attempting to precalculate anything the tabs might need to determine their visibility before calling the iterate_tabs method.  This is not only a little cumbersome--each implementation of can_display has to take a bevy of arguments when it is usually only interested in one or two, it limits extensibility since we might need to add tabs that require information that wasn't known at the time of the original design.

I've left this unresolved, for now, so I can explore with you guys some possible solutions.  Some possibilities include:

- Stay true to the original design and add yet another argument to iterate_tabs and can_display: is_poc_coach.  This is a pretty terrible option, but it is an option.  It means passing around that value to all of the tabs, even though only one tab would actually use it.  

- Clean up the iterate_tabs architecture a little bit and replace the three boolean arguments with a single 'user' argument.  Between the course and the user, the tabs code then has everything it needs to calculate can_display for each tab.  This makes the signatures cleaner for iterate_tabs and can_display and promotes extensibility by providing all the information any new tabs could need in the future.  I actually made this change on my local copy and wound up reverting it because to reimplement the existing tabs I needed to import courseware.access.has_access and student.models.CourseEnrollment.  I know that conceptually, since the tabs code is in xmodule, these imports are in the wrong direction.  You want xmodule code to be a dependency of application code, not vice versa.  And the real world consequence of this is that I hit circular imports.  There are ways to work around circular imports, but it was getting just a little too invasive and weird for me to feel comfortable making that kind of change unilaterally.

Now, in my opinion, the tabs code is in the wrong place anyway.  It really belongs in courseware.  It is application level code, not framework level code.  If it were up to me I'd move it and change the signatures around as described.  And find out the unintended consequences the hard way.  ;)  

- I could just place a call to courseware.courses.get_request_for_thread() to get the current request, get the user from that, and then check the user's role in the course.  This is a bit of a hack, but ultimately much easier than the second way.  There are still imports going the wrong direction, but at least they're confined to one very specific place in the code.  And again, I think the issue of wrong direction imports is symptomatic of the tabs code being in the wrong place to begin with.  With lack of buy in for something like the second method above, I'll probably just do this.  It's kind of an ugly hack, but it's an easy one, and it allows me to move on.

- Some fourth way I haven't thought of yet.  Suggestions welcome.

Input and advice is appreciated.  I think the second option is the "right" way but if folks aren't feeling that then I'll just use the third way as a fallback.

Thanks!

Chris

[Piotr Mitros]

Now, in my opinion, the tabs code is in the wrong place anyway.  It really belongs in courseware.  It is application level code, not framework level code.  If it were up to me I'd move it and change the signatures around as described.  And find out the unintended consequences the hard way.  ;)  

This is slightly tangential to the top-level question, but getting there might be less work than you might think. One thing which might be of interest is the profile tab in edX insider. It's a top-level XBlock: 

  https://github.com/pmitros/edX-Insider

The proposed path to top-level XBlocks is: 

Step 1 (complete): Allow XBlocks to disable/enable chrome (accordion, tabs). This is needed regardless for things which need more screen real estate. Allow XBlocks to hide from the accordion as well (which we need for things like 6.002x tutorials). 

Step 2 (complete): Allow XBlocks to override where they are in the navigation structure. An XBlock can specify it is in a specific tab. 

At this point, we can point a new tab to a hidden XBlock with chrome disabled (this is done through a hard URL right now, which is a hack), giving a top-level XBlock. This is how Insider does this. From here, what remains is: 

Step 3 (not started): Allow tabs to point to XBlocks in a course (rather than just hardlink to URLs)

Step 4 (not started): Rejiggle course.xml to allow there to be a <tabset> top level element. This shows the tabs on top, and through inheritance, sets where the children appear in the navigation. If this is not there, this would default to the existing behavior, at least for a while. 

Step 5 (not started): Studio authoring.

Step 6 (not started): Convert existing tabs to XBlocks. 

If this were done, the platform would be substantially more architecturally coherent, and, as you suggested, this would move the code into the courseware, rather than framework-level. Andy Armstrong has been thinking about this, and there has been talk about doing a bit of work around this at the Open edX hackathon next week. If you wanted to participate, you could see whether this is something he's still pursuing. 

Piotr

[Andy Armstrong]

Hi Chris,

As Piotr mentioned, we have been thinking about how best to improve the way tabs are handled in edX. We don't have anything very concrete that would help you out in the short term, so I think your best bet is to carry on as you were thinking. Of your options, I agree that option 2 is the cleanest and the most powerful. There are many reasons why we might need to have the display of the tabs be affected by the current user, so I'm in favor of you going ahead and fixing it.

What I'd recommend is that you make a separate pull request to just change the signature of the method and all the usages. That way it can get into the code base quickly without getting tied up in your longer term project. I'm also very interested in what you have planned for POCs if you want to discuss that in more detail too.

Thanks,

 - Andy

--

Andy Armstrong

edX | UI Architect  | an...@edx.org  

141 Portland Street, 9th floor

Cambridge, MA 02139

http://www.edx.org

[Chris Rossi]

On Monday, November 17, 2014 11:57:17 AM UTC-5, Andy Armstrong wrote:

Hi Chris,

As Piotr mentioned, we have been thinking about how best to improve the way tabs are handled in edX. We don't have anything very concrete that would help you out in the short term, so I think your best bet is to carry on as you were thinking. Of your options, I agree that option 2 is the cleanest and the most powerful. There are many reasons why we might need to have the display of the tabs be affected by the current user, so I'm in favor of you going ahead and fixing it.

What I'd recommend is that you make a separate pull request to just change the signature of the method and all the usages. That way it can get into the code base quickly without getting tied up in your longer term project. I'm also very interested in what you have planned for POCs if you want to discuss that in more detail too.

Thanks Andy,

If the architecture is going to be changing again anyway, is 2) more trouble than it's worth?  If you guys don't think so, I'm happy to go ahead and do it.  There will still be a little messiness from imports at function scope to avoid circular imports at module scope.

Thanks,

Chris

[Andy Armstrong]

Good question! I'm not sure when the architecture will be changing, and when/if it does it will need to remain backward compatible. My recommendation would be to go ahead and make your changes now because there is no clarity as to where we might take things in the future.

 - Andy

[Peter Pinch]

I'm bumping this thread back up to see if there has been any further thinking since mid-November.

We are looking for a way to hide & show tabs based on user role (to be specific, we're adding a coach role and a corresponding coach tab; and we would like to hide the discussion tab for certain users).

In November, the answer was "option 2" which I believe is the following:

Clean up the iterate_tabs architecture a little bit and replace the three boolean arguments with a single 'user' argument.  Between the course and the user, the tabs code then has everything it needs to calculate can_display for each tab.  This makes the signatures cleaner for iterate_tabs and can_display and promotes extensibility by providing all the information any new tabs could need in the future.  I actually made this change on my local copy and wound up reverting it because to reimplement the existing tabs I needed to import courseware.access.has_access and student.models.CourseEnrollment.  I know that conceptually, since the tabs code is in xmodule, these imports are in the wrong direction.  You want xmodule code to be a dependency of application code, not vice versa.  And the real world consequence of this is that I hit circular imports.  There are ways to work around circular imports, but it was getting just a little too invasive and weird for me to feel comfortable making that kind of change unilaterally.

Is this still the right way to go?

Thanks,
Peter

[Andy Armstrong]

Hi Peter,

I have just started thinking more about this area again as we've added one tab ("Notes") recently and will be adding at least one more shortly ("Teams" or maybe "Groups"). I would like to get to the point where new tabs can be provided cleanly by any Django app without changing the core platform, which at first blush means that we should create a plugin interface (using Stevedore, most likely). If we build this interface cleanly, then there wouldn't be a need for circular references, but it may take some untangling to get the code into a usable state. The last Stevedore plugin we added had all the same issues that you and Chris mention, and we ended up leaving some hacked circular references just to get something working.

I haven't had time to look at your pull request yet, but I should be able to get to it this week. I'm fine with you using options 2 or 3 (most definitely not 1!). 

I hope this helps. 

 - Andy

[Peter Pinch]

Thanks Andy. 

Do you think it makes sense to submit this in a separate PR (separate from https://github.com/edx/edx-platform/pull/6636) ?

I am inclined to go with option 3 (just place a call to courseware.courses.get_request_for_thread() ) since it doesn't sound like this will be a permanent solution. But I'll have to see what Chris Rossi's thoughts are. 

- Peter

<ABD3F66D-F08D-47D8-96DE-EFC518DC8127[52].png>

[Andy Armstrong]

If you want to do a clean up, then I would prefer a separate PR. However, if you go with option 3 then you can keep it with 6636.

 - Andy

[Chris Rossi]

Hi Peter, et al,

I have a vague recollection of having actually tried #3 after sending that email, and found that: 1) Introspecting the call stack is really expensive, and 2) there are some cases where the 'request' object it finds is not really the one we want.

It occurred to me today, though, that if we just want a quick way to make it work, as opposed to getting into a whole refactor of how the tabs work as part of the scope for CCX (formerly POCs), we could just do it the same way we handle 'get_current_poc' (soon to be 'get_current_ccx', if not already).  We can just have the poc/ccx middleware determine if the user is a coach and record that state in a global threadlocal that would be accessible by the course tab code.  It is still a hack, but a much cleaner, better performing one, until course tabs get refactored by edX.

Chris