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

531: Optimizing email and issuing task. #632

Closed
wants to merge 1 commit into from

Conversation

ojnadjarm
Copy link
Contributor

No description provided.

@ojnadjarm
Copy link
Contributor Author

Hi @mdjnelson, this is the PR, Claud's changes are pretty much the same as some of the Mohamed code. We are good with this I guess. Let me know what you think.

@mdjnelson
Copy link
Owner

mdjnelson commented Jul 23, 2024

Hi @ojnadjarm, thanks for this. Have you seen the comments I left on Mohamed's PR at #610? There are a few things I would like addressed.

Also please prepend #531 to your commit message so people know that it is related to a current issue.

@ojnadjarm ojnadjarm changed the title Optimizing email and issuing task. 531: Optimizing email and issuing task. Jul 23, 2024
@ojnadjarm
Copy link
Contributor Author

@mdjnelson Yes, I think I addresed the comments you did on the #610 PR. I guess is about the settings mostly? I do found both valuable, the invisible courses, ppl do weird stuff and I do think a even with a course/category hidden the user may still want to send the certificates, so give the option to do so I don't think is harmful, also the expiration I think is necesary, we needed a way to skip really old certificates like a course with 2 years old endate or a certificate that hasn't issue in a while 1 or 2 years, the user will decide that, yet we are processing them on the loop and overcharging the task. Those are the main issues I could think of, there are another issues you are refering to?

@mohamedmohamedatia
Copy link
Contributor

Hi Mark, ojnadjarm

I apologize for the delay in responding as I was on an extended leave for nearly two months and did not review emails during that time.

Regarding the hidden categories and courses, I configured them to allow administrators to tailor settings according to their organization's needs. Different organizations have varied operational methods. For instance, some universities have between 200K to 500K enrollments per term and retain courses on Moodle for 3 to 10 years. These older courses are usually hidden and are rarely accessed for certificate issuance. Therefore, it might not be efficient to include these in certificate checks for the entire 10 years. Allowing administrators to manage this through configurations ensures flexibility and efficiency.

Similarly, for old courses, I provided options for administrators to decide the time frame for system checks. For example, the system could be set to check only the courses from the last six months. If there is a need to process older courses, administrators can adjust the configurations as necessary.

I hope this addresses your concerns regarding the settings. I will review all the conversations I missed and get back to you.

@mdjnelson
Copy link
Owner

mdjnelson commented Aug 18, 2024

Are you able to rebase these changes on the latest MOODLE_404_STABLE branch please? This branch contains the changes #610 introduced as well as some minor tweaks by myself.

@mdjnelson
Copy link
Owner

Pinging @ojnadjarm. I'd really like to get this in before I do my next release so are you able to do that rebase for me? :)

@ojnadjarm
Copy link
Contributor Author

Hi Mark sure, on it.

@ojnadjarm
Copy link
Contributor Author

Hi @mdjnelson, I wasn't using the table, so my PR modified the install.xml and the install.php was deleted. I'm not sure, if you are ok with that. Review the PR and let me know how do you want me to manage that. I'm using get_config('customcert', 'certificate_offset') and // When we get to the end of the list, reset the offset. set_config('certificate_offset', !empty($customcerts) ? $offset + $certificatesperrun : 0, 'customcert'); for that, that's why I deleted the install.xml part and the install.php before you updated the plugin with your changes too, so I'm not sure this part is good for you.

@ojnadjarm
Copy link
Contributor Author

ojnadjarm commented Aug 22, 2024

All the tests seems to be running just fine along with the one I added for the adhoc task after the rebase. Lmk what you think.

@mdjnelson
Copy link
Owner

Hi @mdjnelson, I wasn't using the table, so my PR modified the install.xml and the install.php was deleted. I'm not sure, if you are ok with that. Review the PR and let me know how do you want me to manage that. I'm using get_config('customcert', 'certificate_offset') and // When we get to the end of the list, reset the offset. set_config('certificate_offset', !empty($customcerts) ? $offset + $certificatesperrun : 0, 'customcert'); for that, that's why I deleted the install.xml part and the install.php before you updated the plugin with your changes too, so I'm not sure this part is good for you.

Hi @ojnadjarm. It would be good if you could delete the commit in your branch that Mohamed created as that is now in MOODLE_404_STABLE. If you pull down the latest changes of MOODLE_404_STABLE then cherry-pick your commits on top from this branch (ignoring Mohameds) then pushed that then that would be great. :)

@ojnadjarm
Copy link
Contributor Author

Hi @mdjnelson I have clean up my PR quite a bit, squash commits into 1 and delete any unnecesary changes. lmk what you think.

@mdjnelson
Copy link
Owner

Thanks @ojnadjarm. Ill look at this on the weekend.

@ojnadjarm
Copy link
Contributor Author

Hi @mdjnelson, I notice that some merge was made over the weeked, already rebased :D

…to improve performance

DEF-2908: Improving code a little bit

DEF-2908: Fixing pipelines and adding adhoc task test

DEF-2908: Fixing pipeline error

DEF-2908: Fixing unnecesary changes

DEF-2908: Fixing test
@mdjnelson
Copy link
Owner

Ive included this in my recent updates, thanks!

@mdjnelson mdjnelson closed this Sep 11, 2024
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.

3 participants