Skip to content
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

Unrevealed module model solutions do not give a clear reason for error #1247

Closed
lainets opened this issue Sep 1, 2023 · 4 comments
Closed
Assignees
Labels
area: user interface User interface issues that are not specifically about navigation or user experience (UX) area: UX student User experience and usability for students effort: days Estimated to take less than one week, from the creation of a new branch to the merging experience: beginner required knowledge estimate requires: priority Currently using this label to flag issues that need EDIT decision ASAP (even if there was priority) type: feature New feature or change to a feature

Comments

@lainets
Copy link
Contributor

lainets commented Sep 1, 2023

When accessing an unrevealed chapter (a model solution chapter whose module hasn't been passed) two things may happen:

  1. If the user isn't logged in on a course that is visible to everyone, the user is given a login screen without any explanation. The user can however access other chapter without issue as the course is visible to everyone.
  2. If the user is logged in, they are given a 403 access restricted page without an explanation that they need to first pass the required module.

The user should be given an explanation in both cases. A login screen should still be shown in the first as the user might have an account but just hasn't logged in.

@lainets lainets added type: feature New feature or change to a feature area: user interface User interface issues that are not specifically about navigation or user experience (UX) area: UX student User experience and usability for students effort: days Estimated to take less than one week, from the creation of a new branch to the merging experience: beginner required knowledge estimate requires: priority Currently using this label to flag issues that need EDIT decision ASAP (even if there was priority) labels Sep 1, 2023
@jsorva
Copy link

jsorva commented Sep 1, 2023

I think this is related to — or a specific instance of — a broader (unreported?) issue: pages that you have no right to access just dump you onto the login screen, and this can happen in a pretty annoying way.

For example, let’s say that I’m a random visitor browsing O1’s pages (without being logged in or perhaps even having an account). I’m currently in Chapter 1.6 and using the page-top navigation to move across chapters.

I click on Chapter 1.7 to move there → OK.
I click on Chapter 1.8 → OK.
Then it’s the Week 2 ToC → OK.
Then to Chapter 2.0 (the weekly bulletin page that is only for enrollees) → BLAM! I’m on the login page with no explanation and the navigation is gone.

@markkuriekkinen
Copy link
Contributor

Permissions that are constructed with AND or OR operations from Django REST framework permissions seem to have had this issue for a long time now. The combined OR permission does not have the message attribute from the original permissions, thus A+ loses the intended error message for the permission and the user is not shown any explanatory error messages.

ExerciseVisiblePermission = ExerciseVisiblePermissionBase | JWTExerciseReadPermission

message = getattr(permission, 'message', None)

class ExerciseVisiblePermissionBase(ObjectVisibleBasePermission):
message = _('EXERCISE_VISIBILITY_PERMISSION_DENIED_MSG')
model = LearningObject
obj_var = 'exercise'
def is_object_visible(self, request, view, exercise): # pylint: disable=arguments-renamed
"""
Find out if Exercise (LearningObject) is visible to user
"""
if view.is_course_staff:
return True
user = request.user
if isinstance(user, GraderUser):
return False
if exercise.status == LearningObject.STATUS.HIDDEN:
self.error_msg(_('EXERCISE_VISIBILITY_ERROR_NOT_VISIBLE'))
return False
if (
exercise.is_submittable
and not exercise.course_module.have_exercises_been_opened()
and exercise.status not in (
LearningObject.STATUS.ENROLLMENT,
LearningObject.STATUS.ENROLLMENT_EXTERNAL,
)):
self.error_msg(
format_lazy(
_('EXERCISE_VISIBILITY_ERROR_NOT_OPEN_YET -- {}'),
exercise.course_module.opening_time
)
)
return False
if exercise.audience == LearningObject.AUDIENCE.REGISTERED_USERS:
if not exercise.course_instance.is_student(user):
self.error_msg(_('EXERCISE_VISIBLITY_ERROR_ONLY_RESISTERED_USERS'))
return False
elif exercise.audience == LearningObject.AUDIENCE.INTERNAL_USERS:
if (not exercise.course_instance.is_student(user)
or user.userprofile.is_external):
self.error_msg(_('EXERCISE_VISIBLITY_ERROR_ONLY_INTERNAL_USERS'))
return False
elif exercise.audience == LearningObject.AUDIENCE.EXTERNAL_USERS:
if (not exercise.course_instance.is_student(user)
or not user.userprofile.is_external):
self.error_msg(_('EXERCISE_VISIBLITY_ERROR_ONLY_EXTERNAL_USERS'))
return False
if not exercise.can_be_shown_as_module_model_solution(user):
self.error_msg(_('MODULE_MODEL_ANSWER_VISIBILITY_PERMISSION_DENIED_MSG'))
return False
return True
ExerciseVisiblePermission = ExerciseVisiblePermissionBase | JWTExerciseReadPermission

@jsorva
Copy link

jsorva commented Sep 30, 2023

I think this is related to — or a specific instance of — a broader (unreported?) issue: pages that you have no right to access just dump you onto the login screen, and this can happen in a pretty annoying way.

For example, let’s say that I’m a random visitor browsing O1’s pages (without being logged in or perhaps even having an account). I’m currently in Chapter 1.6 and using the page-top navigation to move across chapters.

I click on Chapter 1.7 to move there → OK. I click on Chapter 1.8 → OK. Then it’s the Week 2 ToC → OK. Then to Chapter 2.0 (the weekly bulletin page that is only for enrollees) → BLAM! I’m on the login page with no explanation and the navigation is gone.

Addendum:
The above scenario describes a visitor who is not logged in. A similar thing happens now with logged-in students who have a deadline extension and are forbidden from viewing a chapter that contains solutions (until after their personal deadline has passed). They are now prevented from seeing the page, which is great, but the info on that page could be better, and, more importantly, it would be very nice to still have the navigation controls there in this case.

markkuriekkinen added a commit that referenced this issue Oct 29, 2023
When a permission check fails, the permission's message attribute
is used as an error message to the user. However, when permission
classes are formed by combining other permission classes with
the `OR`, `AND` or `NOT` operators, then the new class does not
include the message attribute from the original permissions.

This commit detects combined permission classes when
the error message is about to be rendered. The message is then
taken from the first operand permission.

As an example of combined permissions, `ExerciseVisiblePermission`
is combined with `OR` from `ExerciseVisiblePermissionBase` and
`JWTExerciseReadPermission`.

Related to #1247
markkuriekkinen added a commit that referenced this issue Oct 29, 2023
When a course instance is public (visible without authentication),
unauthenticated users used to be able to view chapters that should
be hidden based on the course module model solution reveal rule.
(This also required that the model solution chapter does not have
the audience setting that requires users to authenticate.)
This issue is fixed by checking the course module model solution
reveal rules in `CachedPoints` even when the user is unauthenticated.
In practice, the code only had to be changed so that
the module list variable is not set to the empty list when
the user is unauthenticated. One function definition also had to
be moved out from an if branch.

The scenario described in the issue #1247 was effectively already
fixed in the previous commit that fixed the permission error
messages for permissions that are combined from other permissions
with the `OR` operator. This commit focuses on the visibility bug
described above.

Fixes #1247
@markkuriekkinen
Copy link
Contributor

This was fixed in the v1.20_stable branch in the three commits listed below. The fix needs to be still ported to the master branch.

@github-project-automation github-project-automation bot moved this from In Progress to Done in A+ sprints Oct 30, 2023
murhum1 pushed a commit to murhum1/a-plus that referenced this issue Dec 21, 2023
When a permission check fails, the permission's message attribute
is used as an error message to the user. However, when permission
classes are formed by combining other permission classes with
the `OR`, `AND` or `NOT` operators, then the new class does not
include the message attribute from the original permissions.

This commit detects combined permission classes when
the error message is about to be rendered. The message is then
taken from the first operand permission.

As an example of combined permissions, `ExerciseVisiblePermission`
is combined with `OR` from `ExerciseVisiblePermissionBase` and
`JWTExerciseReadPermission`.

Fixes apluslms#1247
ihalaij1 pushed a commit to ihalaij1/a-plus that referenced this issue Dec 22, 2023
When a permission check fails, the permission's message attribute
is used as an error message to the user. However, when permission
classes are formed by combining other permission classes with
the `OR`, `AND` or `NOT` operators, then the new class does not
include the message attribute from the original permissions.

This commit detects combined permission classes when
the error message is about to be rendered. The message is then
taken from the first operand permission.

As an example of combined permissions, `ExerciseVisiblePermission`
is combined with `OR` from `ExerciseVisiblePermissionBase` and
`JWTExerciseReadPermission`.

Fixes apluslms#1247
ihalaij1 pushed a commit to ihalaij1/a-plus that referenced this issue Dec 22, 2023
When a permission check fails, the permission's message attribute
is used as an error message to the user. However, when permission
classes are formed by combining other permission classes with
the `OR`, `AND` or `NOT` operators, then the new class does not
include the message attribute from the original permissions.

This commit detects combined permission classes when
the error message is about to be rendered. The message is then
taken from the first operand permission.

As an example of combined permissions, `ExerciseVisiblePermission`
is combined with `OR` from `ExerciseVisiblePermissionBase` and
`JWTExerciseReadPermission`.

Fixes apluslms#1247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: user interface User interface issues that are not specifically about navigation or user experience (UX) area: UX student User experience and usability for students effort: days Estimated to take less than one week, from the creation of a new branch to the merging experience: beginner required knowledge estimate requires: priority Currently using this label to flag issues that need EDIT decision ASAP (even if there was priority) type: feature New feature or change to a feature
Projects
Status: Done
Development

No branches or pull requests

3 participants