-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add Rubyric exercises, details about the configuration of the exercises and instructions about local development #25
Add Rubyric exercises, details about the configuration of the exercises and instructions about local development #25
Conversation
dbc98d1
to
8f33048
Compare
8f33048
to
9fb16d9
Compare
9fb16d9
to
4ecd9a9
Compare
This PR had something weird in it, and it added some other commits belonging to Jaakko and Saara. Anycase, now the PR is clean :) |
4ecd9a9
to
c28abe3
Compare
@@ -0,0 +1,17 @@ | |||
|
|||
_external: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this internal value required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question. I was actually taking some config values from the o1 course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a-plus-rst-tools adds _external: true
for LTI exercises, but I am not sure if it is really necessary. However, we should recommend teachers to use RST when it is possible instead of writing YAML config files manually. The group size settings are still missing from the submit RST directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely the parameter _external
is not necessary in this case. Therefore, I will remove it. @Mankro which group size setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you can see in a-plus-rst-tools/toc_config.py
that the _external
field is used in an if branch. Thus, it can't be removed.
The group size settings consist of min_group_size
and max_group_size
. They haven't been added to the submit RST directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I said. Nobody has added the group size settings to the submit RST directive. They are missing. Adding them is a task for another pull request. Currently, one needs to use a config.yaml file in order to define a group exercise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I said that the group size settings are missing from the RST submit directive, I meant that they are a missing feature and not YET implemented in the code. You seem to have read it as "missing from the RST example, let's add the options to the example exercise". Now it should be clear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, now it is clear.
exercises/hello_rubyric/config.yaml
Outdated
lti: Rubyric | ||
lti_context_id: '' | ||
lti_resource_link_id: hello-world | ||
lti_open_in_iframe: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work without the iframe as there is the aplus_get_and_post
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so. I think I got lost in the documentation because there is no mention about these two options being somehow mutually exclusive. Maybe we need to add some more details to the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They shouldn't be exclusive, but if the communication is between A+ and the browser, then the iframe shouldn't be requires. But if it communicates directly with rubyric, then it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having both lti_open_in_iframe
and lti_aplus_get_and_post
true does not make sense. It is interesting to see what happens then, though. Since lti_aplus_get_and_post
changes the protocol and it is not LTI at all anymore, there is no request from the browser to the Tool Provider that could be opened in an iframe.
Note that Rubyric requires the aplus get and post setting. Rubyric does not support real LTI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we need to fix that part in the aplus-manual too. You could do it in this same pull request, but keep the git commit separate from the other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure where you took that screenshot. If it was in this same Rubyric module, then ignore my comment about separating the git commits. That wouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fine-tune the details.
You need to rebase your branch since the upstream master was force-pushed in the summer. Your branch has my one old commit that was changed in the force push.
You have examples of both pure RST exercise configuration and manual config.yaml file configuration for Rubyric. We can keep both, but would you emphasize that RST is preferred if the teacher has no special reason to manually write the config.yaml file?
@@ -0,0 +1,17 @@ | |||
|
|||
_external: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a-plus-rst-tools adds _external: true
for LTI exercises, but I am not sure if it is really necessary. However, we should recommend teachers to use RST when it is possible instead of writing YAML config files manually. The group size settings are still missing from the submit RST directive.
exercises/hello_rubyric/config.yaml
Outdated
lti: Rubyric | ||
lti_context_id: '' | ||
lti_resource_link_id: hello-world | ||
lti_open_in_iframe: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having both lti_open_in_iframe
and lti_aplus_get_and_post
true does not make sense. It is interesting to see what happens then, though. Since lti_aplus_get_and_post
changes the protocol and it is not LTI at all anymore, there is no request from the browser to the Tool Provider that could be opened in an iframe.
Note that Rubyric requires the aplus get and post setting. Rubyric does not support real LTI.
lti_resource_link_id: hello-world | ||
lti_open_in_iframe: true | ||
lti_aplus_get_and_post: true | ||
url: /aplus_exercise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to mention in the manual that the special URL /aplus_exercise
is specific to Rubyric and it makes Rubyric create the exercise automatically so that the teacher does not need to manually create any exercises in Rubyric beforehand. If the exercises are created beforehand, then the URL must hardcode the id value that can be seen in Rubyric. Likewise, the resource link id could be manually hardcoded in both systems if there is a need for that, but automatic setup should be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a short description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize this before. Have you tested the automatic Rubyric setup (using url /aplus_exercise
) in group exercises? It is very possible that Rubyric does not set the group size automatically and the teacher needs to set it manually in Rubyric after Rubyric has created the exercise automatically. Furthermore, there are plenty of settings in Rubyric (grading rubrics etc.) that certainly can not be set in the A+ course git repo. The reader should be warned about that, especially the group size setting. (The warning should be in the chapter text, not here in config.yaml.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test that. But it seems like the group size is not set automatically. I tried the following configuration
min_group_size: 1
max_group_size: 4
But the result was the following: 🤔
Screenshot taken from Rubyric
So it seems like those options are useless. Should I remove them from the example?. In one comment above, I added a screenshot that shows that the :min_group_size:
and :max_group_size:
options cannot be added to the RST directive because it causes compilation errors.
Regarding the other settings, I think the whole module cover most of them, and there is no need for adding more details about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The group size settings are NOT useless. They are simply not automatically forwarded to Rubyric and we should remind the reader that many settings need to be manually configured in Rubyric (after the exercise has been created in Rubyric either manually or automatically). Note that the group size setting must be defined in BOTH A+ and Rubyric in order to enable group submissions (when Rubyric exercises are integrated into A+). The config.yaml group size settings define them for A+, and once the group size options are added to the RST submit directive, RST can be used too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the instructions about groups in Rubyric https://plus.cs.aalto.fi/aplus-manual/experimental/rubyric/04_roles_and_groups/#groups do not mention the A+ side. I think we should make those changes in a new PR.
rubyric/02_getting_started.rst
Outdated
|
||
If you want to test Rubyric locally, you must read the | ||
`Official Rubyric documentation <https://github.com/Aalto-LeTech/rubyric/blob/master/doc/rubyric.md#connect-local-rubyric-to-a-course-in-docker-optional>`_ | ||
and follow the instructions on how to use a local copy of Rubyric along with A+. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running Rubyric locally would be much easier if we created a Docker container for it, but that has not been done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but it was slightly difficult, maybe I will try again in the future.
c28abe3
to
11669a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just small changes still needed.
lti_resource_link_id: hello-world | ||
lti_open_in_iframe: true | ||
lti_aplus_get_and_post: true | ||
url: /aplus_exercise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize this before. Have you tested the automatic Rubyric setup (using url /aplus_exercise
) in group exercises? It is very possible that Rubyric does not set the group size automatically and the teacher needs to set it manually in Rubyric after Rubyric has created the exercise automatically. Furthermore, there are plenty of settings in Rubyric (grading rubrics etc.) that certainly can not be set in the A+ course git repo. The reader should be warned about that, especially the group size setting. (The warning should be in the chapter text, not here in config.yaml.)
I think the instruction on how to configure Rubyric using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final changes and then you can squash the git commits. The example exercise and the local installation could be in different commits.
New instruction on how to run Rubyric are added, including links to the updated Rubyric documentation.
A new example using a **config.yaml** file was added to the Manual. This new exercise will allow the users of this manual to be aware of the additional options that can be set for the Rubyric exercises. A short paragraph was also added to describe in some of the options in more detail.
ce3b44d
to
a3e40d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
You had some thoughts about further changes to clarify some settings. Did you write them down?
Description
Instructions about testing Rubyric in a local environment is added.
In addition, one more Rubyric exercise is added to the manual along
with some detail about some of the configuration parameters.
Testing
I tested the exercises in http://localhost:8000/def/current/rubyric/example/
Is it Done?
Clean up your git commit history before submitting the pull request!