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

Added availability_mobileapp UCSFCLE_403_STABLE #113

Conversation

lbailey-ucsf
Copy link
Collaborator

The official release of the Moodle Mobile app availability plugin appears to have not been updated in the past 5 years. Due to open issues in the main branch of the official release, we opted to fork, fix, and deploy our fork for a '4.0' release.

Our forked version passed the phpUnt tests. However, "There were 2 risky tests" reported due to deprecation. PR reviewers can decided whether or not we should address the 'risky tests' prior to integration into our UCSFCLE_404_STABLE branch.

@lbailey-ucsf lbailey-ucsf requested review from ctam and mirleu November 6, 2024 18:45
@ctam
Copy link

ctam commented Nov 7, 2024

However, "There were 2 risky tests" reported due to deprecation.

Was this issue reported to the Moodle HQ, i.e. was an issue created on the moodlehq/moodle-availability_mobileapp repo? If not, could we create one? And/Or create an Asana ticket for this as well, so we bring this to the surface. If a developer has a free cycle, they could fix it before the next Moodle upgrade.

@lbailey-ucsf
Copy link
Collaborator Author

However, "There were 2 risky tests" reported due to deprecation.

Was this issue reported to the Moodle HQ, i.e. was an issue created on the moodlehq/moodle-availability_mobileapp repo? If not, could we create one? And/Or create an Asana ticket for this as well, so we bring this to the surface. If a developer has a free cycle, they could fix it before the next Moodle upgrade.

@ctam

The "risky tests" was based on a single issue that occurred twice (you can see the full trace in the job run):

Deprecated: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /home/runner/work/moodle/moodle/availability/condition/mobileapp/classes/condition.php on line 97

Also, the issue has not been report, but I'll report it. I doubt it will be addressed; there hasn't been any activity on that repo in 5 years.

Note that the plugin is passing core unit tests and Brian reviewed in Tugboat and confirmed the functionality

@lbailey-ucsf lbailey-ucsf marked this pull request as ready for review November 13, 2024 17:14
@lbailey-ucsf
Copy link
Collaborator Author

@ucsf-education/moodlecodereviewers any decision on merging this?

@ctam
Copy link

ctam commented Nov 15, 2024

I've been reviewing this for the last couple of days. And I added a comment to Issue #3 stating the fact that MOODLE_32_STABLE has the latest changes while master is lagging behind. Our UCSFCLE_403_STABLE was based off MOODLE_32_STABLE. So, it is fine that we are using the same UCSFCLE_403_STABLE branch for Moodle 4.4. But it was not obvious to me or anyone at first. I really wish the plugin maintainer would fix this so we can avoid spending time figuring it out every time we work on this plugin.

It looks good and I will approve and merge this.

@ctam ctam merged commit 3850460 into ucsf-education:UCSFCLE_404_STABLE Nov 15, 2024
2 checks passed
@lbailey-ucsf lbailey-ucsf deleted the T-1582/UCSFLCE_404/add-availability_mobileapp branch December 4, 2024 18:32
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