Update Documentation

[93Kamuran]

Pull Request checklist

Please confirm you have completed any of the necessary steps below.

• Meaningful Pull Request title and description
• Changes tested as described above
• Added appropriate documentation for the change.
• Created GitHub issues for any relevant followup/future enhancements if appropriate.

[dmohns]

Hey @93Kamuran

I see you are essentially deleting all the documentation for Environment variables. But I don't understand why 🙈

It looks to me like these are used. And, not only that they are crucial to configuring MPM.

Let's use MAIL_ as an example... As a administrator of an MPM instance, how should I configure the email sending for my instance when not using the corresponding environment variables?

Instead of deleting these could you please instead update the documentation to actually reflect what these environment variables do?

For example SUNKING_API_URL... This ENV is the default URL that get's used when the SUNKING plugin get's enabled. It doesn't look like the user can customise this in the plugin view.

[93Kamuran]

Hey @93Kamuran

I see you are essentially deleting all the documentation for Environment variables. But I don't understand why 🙈

It looks to me like these are used. And, not only that they are crucial to configuring MPM.

Let's use MAIL_ as an example... As a administrator of an MPM instance, how should I configure the email sending for my instance when not using the corresponding environment variables?

Instead of deleting these could you please instead update the documentation to actually reflect what these environment variables do?

For example SUNKING_API_URL... This ENV is the default URL that get's used when the SUNKING plugin get's enabled. It doesn't look like the user can customise this in the plugin view.

We have "mail_settings" table in tenant databases. The missing part is; We need to add one more settings tab for email settings on settings page. (We can not send all emails in the system for all tenants from one email address right?)

If you check the other areas I removed essipecially related plugins like "api url", we have default value on plugin table migrations.

[dmohns]

If you check the other areas I removed essipecially related plugins like "api url", we have default value on plugin table migrations.

Yeah, but the default value is taken from ENV. For example SUNKING: https://github.com/EnAccess/micropowermanager/blob/main/src/backend/database/migrations/micropowermanager/2022_14_12_create_sun_king_tables.php#L13

If the ENV is empty, the table will also be empty. Or am I missing something?

[dmohns]

We have "mail_settings" table in tenant databases. The missing part is; We need to add one more settings tab for email settings on settings page. (We can not send all emails in the system for all tenants from one email address right?)

Mail is a bit more complicated I think. There should be two mail settings.

First one is for instance level email to clients/tenants. I.e. if users sign up so they get Welcome email. This looks like it's actually hardcoded in the code: https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/Helpers/MailHelper.php#L19

Second one, is client emails to customers. I'm not even so sure this is a relevant use-case? I was thinking the client<>customer communication is done via SMS exclusively?

[93Kamuran]

If you check the other areas I removed essipecially related plugins like "api url", we have default value on plugin table migrations.

Yeah, but the default value is taken from ENV. For example SUNKING: https://github.com/EnAccess/micropowermanager/blob/main/src/backend/database/migrations/micropowermanager/2022_14_12_create_sun_king_tables.php#L13

If the ENV is empty, the table will also be empty. Or am I missing something?

solved that with an update.

[dmohns]

solved that with an update.

Unfortunately, it doesn't work like this 🙈

Few things:

• Changing "past" migrations is extremely dangerous it can lead to highly inconsistent situations. We should only do this in exceptional cases.
• In this specific case: When you remove the default value being read from ENV there is no way to use the production URL anymore. If I deploy the prod version, how would I customise it to point to SunKing live env?

Also, the scope of this PR is documentation only. In here, let's try to only document the status-quo. If you want to improve the way these are handled, please send a separate PR for that.

[93Kamuran]

solved that with an update.

Unfortunately, it doesn't work like this 🙈

Few things:

• Changing "past" migrations is extremely dangerous it can lead to highly inconsistent situations. We should only do this in exceptional cases.
• In this specific case: When you remove the default value being read from ENV there is no way to use the production URL anymore. If I deploy the prod version, how would I customise it to point to SunKing live env?

Also, the scope of this PR is documentation only. In here, let's try to only document the status-quo. If you want to improve the way these are handled, please send a separate PR for that.

Changing "past" migrations is extremely dangerous it can lead to highly inconsistent situations. We should only do this in exceptional cases.

 - Thank you for telling this to me :) If I'm not wrong there is no deployed version of the project still. Anyway  we can create an update migration or revert SunKing url in environment_variables.md file. Just tell me your preference please.

Also, the scope of this PR is documentation only. In here, let's try to only document the status-quo. If you want to improve the way these are handled, please send a separate PR for that.

-  So if you choose first option please create a task for this and let me create an update migration.     

[93Kamuran]

We have "mail_settings" table in tenant databases. The missing part is; We need to add one more settings tab for email settings on settings page. (We can not send all emails in the system for all tenants from one email address right?)

Mail is a bit more complicated I think. There should be two mail settings.

First one is for instance level email to clients/tenants. I.e. if users sign up so they get Welcome email. This looks like it's actually hardcoded in the code: https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/Helpers/MailHelper.php#L19

Second one, is client emails to customers. I'm not even so sure this is a relevant use-case? I was thinking the client<>customer communication is done via SMS exclusively?

DESCRIBE mail_settings;

id
mail_host
mail_port
mail_encryption
mail_username
mail_password
created_at
updated_at

Each tenant should specify its on mail settings, for an email which is gonna sent by MPM behalf tenant. MPM needs a email settings page/section also email sending functions should be updated accordingly.

[dmohns]

We have "mail_settings" table in tenant databases. The missing part is; We need to add one more settings tab for email settings on settings page. (We can not send all emails in the system for all tenants from one email address right?)

Mail is a bit more complicated I think. There should be two mail settings.
First one is for instance level email to clients/tenants. I.e. if users sign up so they get Welcome email. This looks like it's actually hardcoded in the code: https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/Helpers/MailHelper.php#L19
Second one, is client emails to customers. I'm not even so sure this is a relevant use-case? I was thinking the client<>customer communication is done via SMS exclusively?

DESCRIBE mail_settings;

id mail_host mail_port mail_encryption mail_username mail_password created_at updated_at

Each tenant should specify its on mail settings, for an email which is gonna sent by MPM behalf tenant. MPM needs a email settings page/section also email sending functions should be updated accordingly.

My question is: Form the code it looks like the mail_settings is not used, but the config object is (and config object will be controlled via env variables. Can you point me to the place where the values from the table are used?

[93Kamuran]

My question is: Form the code it looks like the mail_settings is not used, but the config object is (and config object will be controlled via env variables. Can you point me to the place where the values from the table are used?

Yes, you're right! I re-checked the source code and noticed that we use mailing only for sending emails to tenant admins. and that functions use configs from config/mail.php.

"mail_settings" may stay for tenant databases. We may consider using it in mailing operations between tenant & end customers in future.

So I'm gonna revert my changes related mail envs.

[dmohns]

These seem used.

• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/config/services.php#L35-L40
• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/modules/Transaction/Provider/AirtelTransactionProvider.php#L46-L56

please don't remove them

[93Kamuran]

AirtelTransactionProvider should be removed. We need to develop a plugin for Airtel If we're planning that in future.

[dmohns]

These seem used

• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/config/services.php#L43-L53
• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/ManufacturerApi/CalinReadMeter.php#L29-L35

Please don't remove them

[93Kamuran]

CalinReadMeter.php is not using. We have plugins for Calin & Calin Smart meters. The code sample you shared me is legacy code from very old versions of MPM. So that can be removed.

[dmohns]

These seem used

• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/ManufacturerApi/CalinReadMeter.php#L180-L191

Please don't remove

[93Kamuran]

CalinReadMeter.php is not using. We have plugins for Calin & Calin Smart meters. The code sample you shared me is legacy code from very old versions of MPM. So that can be removed.

[dmohns]

Please also remove the corresponding section in services.php. https://github.com/EnAccess/micropowermanager/blob/main/src/backend/config/services.php#L55-L64

[93Kamuran]

CalinReadMeter.php is not using. We have plugins for Calin & Calin Smart meters. The code sample you shared me is legacy code from very old versions of MPM. So that can be removed.

[dmohns]

These seem used

• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/config/services.php#L22-L33
• https://github.com/EnAccess/micropowermanager/blob/main/src/backend/app/modules/Transaction/Provider/VodacomTransactionProvider.php#L311-L323

Please don't remove them

[93Kamuran]

VodacomTransactionProvider should be removed. We need to develop a plugin for Airtel If we're planning that in future.

[dmohns]

This has to stay for now, see below

[93Kamuran]

ok

[dmohns]

We can only do this, once we have a section in the WebUI for Tenants to set their SunKing URL. Please remove it for now.

[93Kamuran]

This is fixed value, So we need to set it into "sun_king_api_credentials" table with default value once "install plugin" command runs. We do not need to show this information on UI side.

[dmohns]

How would I (as a platform admin) then change my development environment to use the SunKing sandbox?

[dmohns]

Hey @93Kamuran

thanks for your responses. So, my point is: I don't want to remove these from the docs, as long as they are "used" (or mentioned, really) in the code. If I understand your comments correctly none of the code blocks where these appear is actually getting executed? Could you instead then simply remove the code (and the corresponding section of the docs)?