Skip to content

Initial implementation - #1

Merged
itsjeyd merged 32 commits into
masterfrom
initial-implementation
Dec 10, 2015
Merged

Initial implementation#1
itsjeyd merged 32 commits into
masterfrom
initial-implementation

Conversation

@smarnach

Copy link
Copy Markdown
Contributor

ActiveTable XBlock

This XBlock implements a tabular problem type, where students are required to fill in some of the table cells with the correct answers. The score awarded to the student is the proportion of correct answers.

The XBlock is implemented in a straight-forward way. All data is stored in XBlock fields. The content is definied by a table definition in a Python-like syntax and some supporting fields. Student state consists of the answers students gave, the correctness of these answers and the score awarded. The amount of data (both content and student state) is very small (likely to be far less than 1K per problem per student). Since the amount of data is so small, and the XBlock doesn't do anything that is computationally intensive, we don't expect that there are any performance bottlenecks. We haven't performed any load testing.

We'd like to get this XBlock on edx.org for Davidson College by mid-December.

For testing on the devstack, install the XBlock using

pip install -e .

as user edxapp from inside the repo directory and add activetable to the Advanced Module List in your course settings. Most of the features can be tested directly with the example table you get when adding an ActiveTable component to a unit.


Partner information hosted on edx.org (Davidson College)
Merge deadline mid-December
JIRA Story OSPR-923
Confluence / Product Asset N/A
Sandbox URL LMS (Example), Studio (Example)
Dependencies N/A


PR Author(s) Notes / To-Do

  • Fixed: Some integration tests are still missing.
  • Fixed: The images indicating correctness status do not work in Studio, probably because the static URL isn't properly rewritten. The URL rewriting is some undocumented black magic which I didn't manage to debug so far.
  • Implemented the requested feature: I didn't reimplement all the features available in Capa problems, like setting a maximum attempt number or randomization. Is there a way to reuse the Capa implementation instead of reimplementing this stuff?
  • Won't fix: Resetting the user state when logged in as a staff user in the LMS doesn not work for this XBlock. I've got no idea how to fix this.

Screenshots TBD


Reviewers

  • Code: (@Kelketek)
  • Code: (TBD)
  • Product: (TBD)

Comment thread README.md

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're using XBlocks, we can (and should) be designing these sorts of things with Child blocks if we can. This work of doing custom parsing is a lot to maintain when we have things like XBlock-utils to automate much of our form building. If we don't do this now, we're going to be stuck with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kelketek The client explicitly requested to not turn this into a point-and-click interface. See Xavier's comments on the ticket for details.

I think child XBlocks would be a bad fit for this anyway. Making every cell an XBlock would make creating tables really awkward, and it wouldn't be particularly efficient performance-wise either. What I could imagine is to design a tailored graphical interface in JavaScript, but, as mentioned above, we were explicitly asked not to do that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well.

@Kelketek

Copy link
Copy Markdown
Member

@smarnach I'm worried about the approach of parsing the table manually now that we no longer need to.

As for your questions:

I didn't reimplement all the features available in Capa problems, like setting a maximum attempt number or randomization. Is there a way to reuse the Capa implementation instead of reimplementing this stuff?

I don't think so. You will probably have to re-implement it. :/

Resetting the user state when logged in as a staff user in the LMS doesn not work for this XBlock. I've got no idea how to fix this.

This is provided by the XModule shim. I don't think you'll be expected to fix it. You're talking about the button, right? There should still be the possibility of resetting it manually in the Instructor Dashboard, given the problem ID, as I think that's available for all blocks.

The images indicating correctness status do not work in Studio, probably because the static URL isn't properly rewritten. The URL rewriting is some undocumented black magic which I didn't manage to debug so far.

Check how the problem-builder block handles this.

@smarnach

Copy link
Copy Markdown
Contributor Author

@Kelketek Thanks for your suggestions.

I don't think so. You will probably have to re-implement it. :/

There are quite a few Capa features, so I asked Xavier which subset to preserve.

This is provided by the XModule shim. I don't think you'll be expected to fix it. …

Ah, that makes sense. And good to know there's a reset function in the Instructor Dashboard – that's very handy for testing.

Check how the problem-builder block handles this.

Problem Builder doesn't seem to inlcude any static images. After digging a bit further, I found that there is an API method to get resource URLs from the runtime, but edx-platform doesn't support it. I'm now copying the images into the XBlock, but it's still awkward to get the correct URLs in a CSS file.

According to the docstring of the XBlock class, subclasses are not allowed to
override __init__().  (Seriously?)
@smarnach

Copy link
Copy Markdown
Contributor Author

@Kelketek This is ready for another look now. I'll wait for Xavier's answer before implementing any additional Capa features.

@smarnach
smarnach force-pushed the initial-implementation branch from 9ce6a3a to f0d009c Compare October 22, 2015 14:19
Comment thread activetable/activetable.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The row heights and column widths fields have a reset button next to them in the interface. However, this button does not seem to work. Clicking it makes the field appear empty, but saving it does not actually save an empty string-- it just reverts to whatever it was before you hit the button. Putting in a new value does get saved, though.

@Kelketek

Copy link
Copy Markdown
Member

@smarnach This seems good to me so far. Find out from Davidson which of the CAPA features they need reimplemented and make the tests pass-- they're currently failing for a lack of the yaml module. You may wish to mimic problem builder's handling of checks and Xs, etc, but that's not a requirement.

@smarnach

Copy link
Copy Markdown
Contributor Author

@dseaton This XBlock conversion of ActiveTable has all its specific features now. I haven't reimplemented all features that are provided by the JSInput module itself. We don't get these features for free any more. These features include

  1. Setting a maximum number of attempts (and show a "Save" button when a maximum is set)
  2. Optionally show a "Reset" button
  3. Timer between attempts to force students to wait a certain amount of time before retrying
  4. Randomization of problem content
  5. "Show Answer" button (with eight different configuration settings)

Number one would take around 2 hours to implement. Number 2 and 3 would be little effort on top of that, and the last two options would probably take more than two hours each (hard to estimate exactly).

What subset of these features would you like us to reimplement on the XBlock version?

cc @antoviaque

@dseaton

dseaton commented Oct 26, 2015

Copy link
Copy Markdown

@smarnach Good point about the cost of not using JSinput.

  1. Yes.
  2. No, but would we ever need it?
  3. No
  4. No (for now - may need this later)
  5. I think we need a sketch for the hour requirement. Honestly, we only ever use this feature in finished, or attempted mode. Does that simplify? Furthermore, would the answers appear in the table? Or would it be just showing a written solution like in any problem? The latter may be fine.

@antoviaque Let's do an hour review with Julie soon.

Comment thread activetable/activetable.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "No value", say, "Omitting this value"

@nedbat

nedbat commented Dec 2, 2015

Copy link
Copy Markdown

This looks good. I wonder about parsing the table on every action, and whether the UI is right for the course team.

@smarnach
smarnach force-pushed the initial-implementation branch from 1bc4ea3 to 5125445 Compare December 8, 2015 22:16
@smarnach

smarnach commented Dec 8, 2015

Copy link
Copy Markdown
Contributor Author

@cptvitamin I addressed all your comments. The new version is live on the sandbox (linked in the PR description). Could you please take another look? We'd like to get this merged by the end of the week, so I'd appreciate if you could look into this soon.

@smarnach

smarnach commented Dec 8, 2015

Copy link
Copy Markdown
Contributor Author

@nedbat I think I've addressed your comments as well. The course team explicitly asked for the UI to be the way it is now; they specifically didn't want a more graphical interface. Could you please take another look?

@nedbat

nedbat commented Dec 9, 2015

Copy link
Copy Markdown

👍

@nedbat

nedbat commented Dec 9, 2015

Copy link
Copy Markdown

(To clear, my thumbs-up means that I am OK with you merging this code. I believe you have the authority.)

@smarnach

Copy link
Copy Markdown
Contributor Author

@cptvitamin Could you please take another look at this PR? We'd like to get this into Dogwood, so it would be ideal if I could merge it today.

itsjeyd added a commit that referenced this pull request Dec 10, 2015
@itsjeyd
itsjeyd merged commit 1786a2c into master Dec 10, 2015
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.

8 participants