TNL-5621 Fix discussion topics in MIT course. by attiyaIshaque · Pull Request #13662 · openedx/openedx-platform

attiyaIshaque · 2016-10-06T07:17:46Z

TNL-5621

Description

Problem: Courses has many discussion topic duplicates. On loading the discussion topics, it could not find which is a category and which is a subcategory.
Fix: Add entry and subcategory to differentiate the topics either its category or not.

Sandbox

tnl-5621-duplicate.sandbox.edx.org

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

FYI: @awaisdar001

adampalay · 2016-10-13T12:32:45Z

Hey @attiyaIshaque , may you please provide a paragraph about what the original problem was and how this fix fixes it? It's not clear to me just from reading the code.

attiyaIshaque · 2016-10-13T13:12:07Z

@adampalay I have updated the comment.

robrap · 2016-10-13T13:17:01Z

Thanks for tracking this down @attiyaIshaque. I imagine this is the main purpose of adding the type.

Are there any uses of this type in the JavaScript? If so, could you point it out? The reason I ask, is because:
a) The array of tuples in Python gives a slightly more awkward JavaScript data-structure of array of arrays.
b) It may greatly simplify this PR to not have to touch the JavaScript if there is a simple way to separate the array required by Python from the data sent to JavaScript.

Also, was the only problem one of entry vs subcategory? If topic names are repeated at multiple levels of subcategories, will we get a similar type problem, or is this ok?

@robrap Yes, this was the only problem of entry and subcategory.
You can also test this on sandbox: topic names are repeated at multiple levels of subcategories.

This should be using the new Python constant.

robrap · 2016-10-13T13:22:24Z

Based on my other comment, this change may not be necessary. However, if we decide it is needed, I think nameType should be renamed to child, and you could possibly add the comment:

// child[0] is the category name, child[1] is the type (i.e. 'entry' or 'subcategory')

@robrap yeah, it looks good: if we decide type is needed in the javascript.
But my point is here, who is the right person which will be decided type is required or not in the javascript?

Right now I don't see type used in JavaScript. However, rather than removing type, would it be more correct to include the same fix? See the following (untested) code:

var type = child[1]; ... if (_.has(map.entries, name) && type == "entry") { ...

I'm not clear how this is already working without the above.

Note: I don't love the data-structure of a meaningful 2x array for JavaScript, but maybe @andy-armstrong or @bjacobel could weigh in. I guess the alternatives are:

Separate child_name and child_type arrays of equal length, which isn't much better, or

(my preference): a child array of objects/dicts (rather than 2x arrays), with {name: "...", type: "..."}
I think Fix js that prevented single step OE problems (ie only self assessment) ... #2 would look cleaner in JavaScript, but I know it would also affect a lot of js tests.

This is food for thought, unless and until the front-end devs weigh-in.

Hard to say that one data structure is always better than another in all cases, but yes @robrap I think your #2 sounds better:

[ { name: 'foo', type: 'bar', }, { name: 'bizz', type: 'buzz' } ]

That's more JavaScript-y than two arrays, or a 2-D array.

@bjacobel: Thanks. This would result in a lot of changes to test data (see actual review). Could you state whether this should be a requirement for thumbs, or if we should leave this as-is, and simply let it be a lesson learned for the future?

Not a requirement for thumbs (at least from me).

@robrap Are you suggesting we add a TODO here and get this PR going as is?

I had a lot of different comments. My recommendations would be:

Rename nameType to child in all code similar to this code (this seems to be repeated).

Add the child[0] and child[1] comment I detailed above.

Add a check for the type after _.has(map.entries, name) as I documented above.

Forget about changing the data structure. I don't think it is worth adding a TODO that will always be ignored. If you don't do it now, it will not be done.

Hopefully this is clear.

@robrap I have addressed the comments. Kindly review again.
Also, I have updated the sandbox.

robrap · 2016-10-13T13:27:15Z

FYI: @andy-armstrong, @bjacobel: You might have reason to actually want additional data sent to JavaScript for bread-crumbing purposes, and maybe there is a way to solve both issues at once. I'm also curious about our best practices around sending JavaScript exactly what it needs vs the ease of sending everything that is needed either by JavaScript or by Mako.

Use the new constants for 'entry' here, and anywhere server side.

awaisdar001 · 2016-10-13T16:56:43Z

You are converting this to List of Lists here and in lms/djangoapps/django_comment_client/tests/test_utils.py:399 you are making list of tuples. Please use same verywhere.

@awaisdar001: I think the problem here is that tuples make more sense in Python, but get translated to arrays in JavaScript. Switching to a dict would make more sense in both Python and JavaScript, but that would be a larger change. Switching to an array in Python would make this more consistent, but would make less sense in Python. So, if I were going to hold thumbs for consistency, I'd rather see this move to:

children: [ {type: 'entry', name: 'Test Topic'}, {type: 'entry', name: 'Other Topic'}] ],

@awaisdar001: I'm ok with this staying as-is, even though I would prefer to see dict/json used as discussed elsewhere. I'll leave it to you to determine if you want to push for the cleaner code.

If we get the cleaner code (which would read much nicer), I think it would be nice to have type first, as I did above, but that's a nice to have.

@robrap Thank you for following up on the suggestion, I agree that would require a lot of code changes which will go beyond the scope of this change. As it is not the requirement for this change I agree we should move forward as-is.

attiyaIshaque · 2016-10-20T15:46:38Z

@robrap ping.

robrap · 2016-10-20T17:53:31Z

+                            // child[0] is the category name, child[1] is the type (i.e. 'entry' or 'subcategory')
+                            var name = child[0],
+                                entry = entries[name];
                            return subCategoryTemplate({


There's something odd about the names used here. Once you help clarify, it would be great to either fix the names and/or add a comment that provides clarity.

In the other loop over children (see common/static/common/js/discussion/views/discussion_topic_menu_view.js), we use entryTemplate() for entries and categoryTemplate() for subcategories. Here, it looks like these are entries because you are pulling from an entries list, but then you are using a subCategoryTemplate. Can you explain explain this?

Additionally, are all of these children guaranteed to be entries? If so, you should add a comment stating so, and possibly clarify why. If not, then type should be used and you should determine what needs to be done if child[1] is 'subcategory'.

@robrap I have fixed the name from subCategoryTemplate to entryTemplate.
Yes, all of these children would be entries.

robrap · 2016-10-20T17:54:39Z


-                        return HtmlUtils.joinHtml.apply(this, _.map(children, function(name) {
+                        return HtmlUtils.joinHtml.apply(this, _.map(children, function(nameType) {
+                            var name = nameType[0];


This should have the same child rename, and below we should check the `type'.

robrap · 2016-10-20T18:06:41Z

        if entry["sort_key"] is None and sort_alpha:
            entry["sort_key"] = title
-        things.append((title, entry))
+        things.append((title, entry, 'entry'))


I know I didn't mention this earlier, but it would be nice to use constants for these.

robrap · 2016-10-20T18:08:14Z

@attiyaIshaque: I completed another pass. Sorry, I thought I had completed this already.

attiyaIshaque · 2016-10-21T12:35:54Z

@robrap I have addressed the comments.
@adampalay and @robrap Can you please review?

robrap · 2016-10-21T20:55:20Z

-        if name in course_wide_entries:
-            course_wide_children.append(name)
+    for name, c_type in discussion_category_map['children']:
+        if name in course_wide_entries and c_type == "entry":


As discussed, add lms/djangoapps/django_comment_client/constants.py for constants and use them in places like this as well. Add a TODO comment near lms import here that all of course_groups should probably move to lms out of openedx/core.

robrap · 2016-10-21T20:57:11Z

                        topicsJson = {
                            course_wide_discussions: {
-                                children: ['Topic_C_1'],
+                                children: [['Topic_C_1', 'entry']],


You could even use the constant in tests. Should just be search/replace.

@robrap I will add the constants in JS tests also but can you please determine the file in which we can add the constants.

I was thinking about the Python tests.

@andy-armstrong or @bjacobel would be better at suggesting the appropriate place for constants in JavaScript.

We don't have any one defined place for them as far as I know. If you're sharing them between two files a constants.js at the root of whatever directory tree both files share sounds good to me.

robrap · 2016-10-21T21:00:16Z

                     */
                    getCourseWideDiscussionsHtml: function(courseWideDiscussions) {
-                        var subCategoryTemplate = HtmlUtils.template($('#cohort-discussions-subcategory-tpl').html()),
+                        var entryTemplate = HtmlUtils.template($('#cohort-discussions-subcategory-tpl').html()),


As discussed, we shouldn't use both entryTemplate (entry) and #cohort-discussions-subcategory-tpl (subcategory). I think you should discuss with @muzaffaryousaf to better understand the naming of the underscore templates. Maybe there is a special reason that entries are being treated as subcategories in this case.

First clarify understand. Then possibly update names. Maybe entryTemplate is better as subCategoryTemplate even if it contains entries? I don't know at this point.

Once all is clarified, add clarifying comments where appropriate. Possible both in the underscore template(?) as well as here.

I would go for subCategoryTemplate given that structure of discussions category map and reusability only if we don't change the name of template.

The other way is to rename the template name and use entryTemplate for course wide discussions but keep in mind we're using same template in inline template.

It may make sense to go with subCategoryTemplate, but it would be great if someone could help define category, subcategory, and entry. I see from @muzaffaryousaf's link that my attempt to define these was wrong. Are entries leaf subcategories? Other? I'm just not clear on what the names mean at this time.

I think we would go for subCategoryTemplate.
In the inline discussion Type can be "entry" or "subcategory", is actually the category type. So "category" means either an "entry" or "subcategory".
I am hesitant because I do not have the root knowledge about this so I will defer to you in this regard what is the right naming convention can be.

The underlying tree structure of the discussion topics is bit confusing, for course wide topics entries are sub categories when we compare them with inline but on other hand they're categories too since they don't have parent.
I would go for comments or tree structure as an example that can help someone to understand the conventions.

@muzaffaryousaf @attiyaIshaque: Thanks for your patience with this. Hopefully we can help once we are all clear.

@muzaffaryousaf: Can you carefully read the following notes and determine if you think anything is inaccurate?

Course wide discussions: - children are all entries (leaf nodes) Inline discussions: - contains top-level container named subcategories - children can be mix of deeper subcategories or entries (leaf nodes) (in any order?) Other details: - Entries always have an id and represent a discussion topic. - Entries are always leaf nodes, and never have children. - Categories and subcategories are also names used in the UI to differentiate between top-level versus nested categories in inline discussions. - In the UI (and Read the Docs), top-level categories are actually represented as the top-level subcategories data-structure. - In the UI (and Read the Docs), either a category or a subcategory may actually be an entry (leaf node) in the data-structure. The most confusing term is subcategory, because: - A subcategory is sometimes a container of subcategories (everywhere). - A subcategory is sometimes a top-level container (in the UI code and the data-structure, but not the UI or Read the Docs). - A subcategory is sometimes an entry or topic (in the UI code, the UI, and Read the Docs, but not in the data-structure).

Unfortunately, it is out-of-scope to fix all of this confusion as part of this PR.

Here's what I think we should do:

Once @muzaffaryousaf fixes or signs off on my comments above, I will try to document it in Confluence so it is captured somewhere. I will also need to update the Glossary in Confluence which is currently inaccurate.

I would leave the above rename to entryTemplate as-is, which matches the other usage.

I would add the following comment into cohort-discussions-subcategory.underscore:

<% // TODO: This underscore template is a template for entries (leaf nodes), and not for subcategories. // We should rename uses of the term subcategory in this template, since subcategory can mean too // many different things. %>

I would rename cohort-discussions-subcategory.underscore to cohort-discussions-entry.underscore if it doesn't cause much of a problem, since it seems to just be used in 2 places.

Let me know your thoughts.

@muzaffaryousaf: I changed and moved the comment above to a Glossary in Confluence on a related page. Feel free to comment/edit there instead of here.

@attiyaIshaque: This move shouldn't affect my recommendations above. I just got started on bullet 1.

@muzaffaryousaf Ping.

@attiyaIshaque I'll comment on confluence if anything comes up about the comment that @robrap made, i think that has nothing to do with this PR.

robrap · 2016-10-21T21:01:06Z

-                            return subCategoryTemplate({
+                        return HtmlUtils.joinHtml.apply(this, _.map(children, function(child) {
+                            // child[0] is the category name, child[1] is the type (which is always 'entry')
+                            var name = child[0],


If these must only be entries, please add a code comment describing that, and possibly why.

Maybe something more like this:

// child[0] is the category name // for course wide discussions, the type stored in child[1] will always be 'entry'.

Slightly adjusted comment:

// child[0] is the category name, child[1] is the type. // For course wide discussions, the type is always 'entry'.

attiyaIshaque · 2016-11-01T10:06:21Z

@robrap Can you please review again ?

robrap

Minor fixes requested. Did you decide against js constants because you couldn't make it happen, or because the PR has been going too long, or other?

robrap · 2016-11-01T12:00:46Z

# A leaf node in a category hierarchy.

robrap · 2016-11-01T12:01:14Z

# A non-leaf node in a category hierarchy.

robrap · 2016-11-01T12:14:12Z

This should be using the new Python constant.

attiyaIshaque · 2016-11-01T17:29:50Z

@robrap I don't know where to add constant.js file and how to load the new js files.
I think we can leave it as-is.

robrap · 2016-11-01T17:34:11Z

Looks good once quality tests are passing.
UPDATE:
I give thumbs to the code, but I removed thumbs only because I never reviewed on the sandbox. I will not be around until next week, so you may need/want someone else to review on the sandbox. Thanks.

attiyaIshaque · 2016-11-09T12:10:37Z

@adampalay , @Qubad786 can you please review it?

Qubad786 · 2016-11-09T12:54:49Z

-            course_wide_children.append(name)
+    for name, c_type in discussion_category_map['children']:
+        if name in course_wide_entries and c_type == TYPE_ENTRY:
+            course_wide_children.append([name, c_type])


@attiyaIshaque , you also need address this pattern in doc string.

@Qubad786 I will update the doc string. thanks for pointing this out.

@attiyaIshaque , also please see if any other doc string needs same addressing too.

Qubad786 · 2016-11-09T13:51:25Z

Looks good on sandbox too. 👍 after tests pass.

robrap · 2016-11-09T16:14:42Z

👍

alisan617 · 2016-11-14T20:34:47Z

I am seeing js error (Cannot read property 'children' of undefined in discussion_topic_menu_view.js) in Editing Post view in Stage. Could someone verify?

andy-armstrong · 2016-11-14T20:54:35Z

@alisan617 I see the same issue. Please report this as a bug and tag the release room that this is a blocker.

adampalay · 2016-11-14T22:58:04Z

@attiyaIshaque , can you please make a PR against release-candidate to revert this PR? Thanks for catching this, @andy-armstrong and @alisan617 !

alisan617 · 2016-11-15T02:39:30Z

Unfortunately, I think the problem is from my PR #13843. I am in process of reverting to release-candidate.

attiyaIshaque · 2016-11-15T05:58:56Z

@adampalay, @andy-armstrong This error TypeError: Cannot read property 'children' of undefined
is already reported https://openedx.atlassian.net/browse/TNL-5713 before my PR merged.

adampalay · 2016-11-15T14:28:21Z

My bad!

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch 6 times, most recently from 8115376 to f4c5ce6 Compare October 13, 2016 07:14

robrap reviewed Oct 13, 2016

View reviewed changes

awaisdar001 reviewed Oct 13, 2016

View reviewed changes

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch 2 times, most recently from fac61e1 to 08f2f47 Compare October 19, 2016 09:16

robrap reviewed Oct 20, 2016

View reviewed changes

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch from 08f2f47 to 10ca542 Compare October 21, 2016 09:28

robrap reviewed Oct 21, 2016

View reviewed changes

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch from 10ca542 to 5afd614 Compare October 31, 2016 10:29

robrap requested changes Nov 1, 2016

View reviewed changes

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch from 5afd614 to 337001b Compare November 1, 2016 16:28

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch 2 times, most recently from 17c3001 to f14dd92 Compare November 3, 2016 07:18

Qubad786 reviewed Nov 9, 2016

View reviewed changes

robrap approved these changes Nov 9, 2016

View reviewed changes

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch from f14dd92 to f9497e8 Compare November 9, 2016 16:31

TNL-5621 Fix discussion topics in MIT course.

76e1d4a

attiyaIshaque force-pushed the atiya/TNL-5621-Duplicate-discussion branch from f9497e8 to 76e1d4a Compare November 9, 2016 17:27

attiyaIshaque merged commit 9728759 into master Nov 9, 2016

attiyaIshaque deleted the atiya/TNL-5621-Duplicate-discussion branch November 9, 2016 18:30

Uh oh!

Conversation

attiyaIshaque commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Sandbox

Reviewers

Uh oh!

adampalay commented Oct 13, 2016

Uh oh!

attiyaIshaque commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attiyaIshaque Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attiyaIshaque Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attiyaIshaque commented Oct 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap commented Oct 20, 2016

Uh oh!

attiyaIshaque commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

attiyaIshaque commented Oct 6, 2016 •

edited

Loading

attiyaIshaque Oct 14, 2016 •

edited

Loading

attiyaIshaque Oct 18, 2016 •

edited

Loading

muzaffaryousaf Oct 31, 2016 •

edited

Loading