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

Webservice: List issued certificates #644

Open
wants to merge 1 commit into
base: MOODLE_404_STABLE
Choose a base branch
from

Conversation

michallohnisky
Copy link

Resolves #419 .

Use-case example:
An external application (CRM) periodically downloads new certificates and associates them with users using their email addresses. The timecreatedfrom filter is used to download certificates issued after the last synchronization.

@mdjnelson
Copy link
Owner

Hi,

There are a few things that need addressed.

  1. The complaints from the GitHub Actions report.
  2. I support Moodle 4.1 upwards so we should be using type hints, return types etc.
  3. This does not resolve Call list of certificates for a user via API #419 as that specifies getting certificates for a user, not from a date.
  4. Indentation of SQL should be as https://moodledev.io/general/development/policies/codingstyle/sql.
  5. There are no checks to see if the user can actually get the list of certificates meaning anyone can get a list of certificates which is a huge security issue.

Please address these and I will take another look.

@michallohnisky
Copy link
Author

michallohnisky commented Oct 2, 2024

Hi, thanks. I will try my best :-) .

  1. Almost done. They disappeared after my push. Can you run them again?
  2. Can you give me an example? Because for example \mod_customcert\external::list_issues_parameters has a return type in annotations, so it is the same as \mod_customcert\external::delete_issue_parameters
  3. Done. I added additional filters.
  4. I cannot see what is wrong, can you give me an example?
  5. Done. I added mod/customcert:viewallcertificates

@michallohnisky
Copy link
Author

  1. Done.

@mdjnelson
Copy link
Owner

Thanks @michallohnisky. Are you able to add some unit tests to this as well? Take a look at https://github.com/mdjnelson/moodle-mod_customcert/blob/MOODLE_404_STABLE/tests/external_test.php to see other examples. :)

@mdjnelson
Copy link
Owner

Regarding the points -

  1. These web services were created ages ago before type hinting and return types were a thing in PHP.
  2. The SQL indentation was corrected so it's fine now.

Cheers.

P.S Once all done please squash into one commit.

@michallohnisky
Copy link
Author

Are you able to grant me permission to run the checks? If I am able to run them immediately after every push, I think I can add some unit tests as well.

@mdjnelson
Copy link
Owner

You can run the unit tests locally before pushing them. :) https://docs.moodle.org/dev/Writing_PHPUnit_tests

@michallohnisky michallohnisky force-pushed the mod_customcert_list_issues branch from 0f2a9f4 to c6174b6 Compare October 10, 2024 09:44
@michallohnisky
Copy link
Author

I understand :-) , but I would need to set up the whole development stage and Docker containers, and it would take a lot of time for a single unit test.

I squashed the commits.

@mdjnelson
Copy link
Owner

I run the unit tests on my machine and I do not use Docker for Moodle development as I find the Moodle HQ container to be extremely slow. So, its possible to do this without Docker.

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.

Call list of certificates for a user via API
2 participants