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

fix for availability in hidden sections #131

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

Syxton
Copy link
Owner

@Syxton Syxton commented Dec 10, 2024

Possible fix for #130

@Syxton Syxton force-pushed the module-visibility-and-availability branch 2 times, most recently from 35aee3b to 7f4861a Compare December 10, 2024 17:41
@Syxton
Copy link
Owner Author

Syxton commented Dec 10, 2024

@PhMemmel if you have a moment I wouldn't mind a second pair of eyes before I merge this. We were seeing this issue on 4.4+ and 4.5.

@PhMemmel
Copy link
Collaborator

Will have a look as soon as I find some time. Thanks for finding this and providing a fix!

Copy link
Collaborator

@PhMemmel PhMemmel left a comment

Choose a reason for hiding this comment

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

LGTM. This moodle feature is pretty confusing :)

I wonder if there is no moodle core function to handle this, so we do not have to rebuild the logic here, but I assume the answer is "no", otherwise we would already use it :)

I also tested this manually, works as intended, bug fixed.

I would suggest to add this case to the unit test function?

classes/actions.php Outdated Show resolved Hide resolved
classes/actions.php Outdated Show resolved Hide resolved
@Syxton Syxton force-pushed the module-visibility-and-availability branch 2 times, most recently from 3d50043 to da7b6eb Compare December 16, 2024 18:57
@Syxton Syxton force-pushed the module-visibility-and-availability branch from da7b6eb to abf8deb Compare December 16, 2024 18:59
@Syxton Syxton force-pushed the module-visibility-and-availability branch from b8685b2 to e248d01 Compare December 16, 2024 19:47
@Syxton
Copy link
Owner Author

Syxton commented Dec 16, 2024

@PhMemmel ok I have rebased this change to the 4.5 version. I also reviewed the unit tests and it already tested correctly, however what we really needed was a behat test making sure that in all cases it was showing correctly to teachers and students. That is now done. However, in the case that somehow a hidden section module gets set to visible: 1 and visibleoncoursepage: 0, there is still the strange case that Moodle will select "Hidden from students" in the activity edit screen under Availability. Seems like a Moodle bug.

The logic is (visible, visibleoncoursepage).
Visible sections: Visible (1, 1), Hidden (0, ~), Avail (1, 0)
Hidden sections: Hidden (0, ~), Avail (1, 1)

@PhMemmel
Copy link
Collaborator

I retested and to be honest the status under availability is correct for me? Or have I the wrong reproduction steps?

@Syxton
Copy link
Owner Author

Syxton commented Dec 18, 2024

@PhMemmel The steps were (using an older version of mass actions), Create a hidden course section. Place an activity in the hidden section. Use mass actions to make the activity in the hidden section available. It should show available. Edit the activity, and under Common module settings -> Availability it will show "Hide on course page". Another way is to manually set the activity in the hidden section to visible = 1 and visibleincourse = 0.

@PhMemmel
Copy link
Collaborator

I followed your steps. With this branch I cannot reproduce this anymore. It properly shows "available, but don't show on course page" in the activity edit form. So I think it can't become better than the current state it. Ready to merge I believe?

@Syxton Syxton merged commit c373cdf into master Dec 19, 2024
8 checks passed
@Syxton Syxton deleted the module-visibility-and-availability branch December 19, 2024 14:03
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.

2 participants