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

Bulk upload suppress emails #168

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Conversation

matthewhilton
Copy link
Contributor

@matthewhilton matthewhilton commented Jul 3, 2024

Changes

  • Adds a checkbox to suppress emails when bulk uploading, to bring it in alignment with the manual assignment page.
  • A few fixups to the code that were causing debugging messages:
    • Output before redirect
    • Context being set when it didn't need to be (and was trying to set a different context)
    • Undefined var $courseid
    • Undefined var $session
    • Removed unused import

Testing

  • Manual testing - works as expected.
  • Also covered by unit tests

@matthewhilton matthewhilton force-pushed the bulk-upload-suppress-emails branch 3 times, most recently from 23fb14c to 62ea50c Compare July 3, 2024 23:48
Copy link
Contributor

@keevan keevan left a comment

Choose a reason for hiding this comment

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

Overall the changes here looks good. Thanks for adding in the tests.

Are you able to consider the feedback comments and once ready, add a commit to also bump the version.

Also noting that ideally this would land in 403 stable before it is backported to 400. Perhaps we should merge it in that order, if you could prepare a PR for that.

Thanks @matthewhilton.

@matthewhilton matthewhilton changed the base branch from MOODLE_400_STABLE to MOODLE_403_STABLE July 5, 2024 01:42
@matthewhilton matthewhilton force-pushed the bulk-upload-suppress-emails branch from 62ea50c to e1b2639 Compare July 5, 2024 01:42
@matthewhilton matthewhilton force-pushed the bulk-upload-suppress-emails branch from 0a4b75a to 6c032f0 Compare July 5, 2024 01:55
@matthewhilton
Copy link
Contributor Author

Confirming I have rebased and changed the target to MOODLE_403_STABLE

Note - After locally testing this, I found a new error: Exception - Undefined constant "mod_facetoface\form\FILE_INTERNAL" when loading the form to upload the csv.
This seems to be caused by the reordering of the $OUTPUT->header (i.e. it was being implicitly included before). I have added another commit to explicitly include these files as well.

Copy link
Contributor

@keevan keevan left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for making the changes Matt.

@keevan keevan merged commit e7576c1 into MOODLE_403_STABLE Jul 5, 2024
11 checks passed
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