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 an option to set the default notification method for users #1242

Conversation

Andrew-Staves-Activ
Copy link
Contributor

@Andrew-Staves-Activ Andrew-Staves-Activ commented Jul 25, 2022

This applies when a new user registers themselves. It also sets the default value for the notification method in the form for adding a new user from the users admin area.

I have not added an option to core Xoops before, so this may be completely the wrong way to go about it.

To support the code changes, I (manually) ran SQL equivalent to the following against my (Xoops) database:

INSERT INTO `xoops_config` (`conf_modid`, `conf_catid`, `conf_name`, `conf_title`, `conf_value`, `conf_desc`, `conf_formtype`, `conf_valuetype`, `conf_order`)
VALUES (0, 2, 'defaultnotificationmethod', '_MD_AM_DEFAULT_NOTIFICATION_METHOD', '1', '_MD_AM_DEFAULT_NOTIFICATION_METHOD_DESC', 'select', 'int', 3);

INSERT INTO `xoops_configoption` (`confop_name`, `confop_value`, `conf_id`)
VALUES ('_MD_AM_DEFAULT_NOTIFICATION_METHOD_DISABLE', '0', (SELECT `conf_id` FROM `xoops_config` WHERE `conf_name` = 'defaultnotificationmethod')),
('_MD_AM_DEFAULT_NOTIFICATION_METHOD_PM', '1', (SELECT `conf_id` FROM `xoops_config` WHERE `conf_name` = 'defaultnotificationmethod')),
('_MD_AM_DEFAULT_NOTIFICATION_METHOD_EMAIL', '2', (SELECT `conf_id` FROM `xoops_config` WHERE `conf_name` = 'defaultnotificationmethod'));

I have little to no idea how these queries should be represented in the repository (if they should be at all). Please do let me know how I can get these queries into the repository and/or clean up this implementation (as necessary) - thanks!

@Andrew-Staves-Activ
Copy link
Contributor Author

Though I expect this is unfinished, I'm setting it to "Ready for review" as I don't know how to proceed without reviews/advice.

@Andrew-Staves-Activ Andrew-Staves-Activ marked this pull request as ready for review July 25, 2022 05:31
@mambax7 mambax7 requested review from geekwright and zyspec July 25, 2022 05:38
@mambax7
Copy link
Collaborator

mambax7 commented Jul 25, 2022

Richard (aka @geekwright), our Core Team leader, is in process of moving, and will be available in the latter part of August to take a look into this and approve it. I hope, you can be patient till then.
Thanks!

@Andrew-Staves-Activ
Copy link
Contributor Author

Andrew-Staves-Activ commented Jul 25, 2022

Sure, this is a case of trying to cleanly implement something that was previously hacked into a production install some years ago (not by me!). This is my attempt at a cleaner replacement and is what said install is now running. I'm just looking to future-proof this by (hopefully) getting some equivalent functionality into Xoops core. I'm open to advice on cleaning this up further. But there's no rush, it's running fine and is (somewhat) less hacky than when I started 🙂

Copy link
Contributor

@ggoffy ggoffy left a comment

Choose a reason for hiding this comment

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

I have updated transifex

@GregMage
Copy link
Contributor

GregMage commented Mar 5, 2023

Thank you for this proposal. We will study the code in detail for a future version of XOOPS (after the release of XOOPS 2.5.11).

Copy link
Contributor

@geekwright geekwright left a comment

Choose a reason for hiding this comment

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

What is missing is the creation of the entries for the config and options in install/page_tablesfill.php

Also, needs a check in the upgrade code that adds the config and options, if needed.

Both of these are the in the more obscure parts of Xoops.

@mambax7
Copy link
Collaborator

mambax7 commented Nov 2, 2023

I'll take care of it

@mambax7 mambax7 requested a review from geekwright November 2, 2023 09:18
@mambax7 mambax7 force-pushed the default_notification_method_option branch from a73cb1b to 84f07ff Compare November 2, 2023 12:58
@mambax7 mambax7 requested review from ggoffy and GregMage November 2, 2023 13:11
@mambax7
Copy link
Collaborator

mambax7 commented Nov 4, 2023

was anybody else able to test it? I would like to commit it for RC3

Copy link
Contributor

@geekwright geekwright 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 to me. Thanks!

@geekwright geekwright merged commit 838aafa into XOOPS:master Nov 7, 2023
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.

5 participants