Skip to content
This repository has been archived by the owner on Aug 9, 2021. It is now read-only.

Patch for PR of OS min/max versions #527

Merged
merged 3 commits into from
May 25, 2018

Conversation

DIOHz0r
Copy link
Contributor

@DIOHz0r DIOHz0r commented May 23, 2018

Changes description

Add missed code for the PR #509

accesslint[bot]
accesslint bot previously approved these changes May 23, 2018
Copy link
Contributor

@btry btry left a comment

Choose a reason for hiding this comment

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

this PR can actually be closed without merge

@@ -43,7 +43,7 @@
'type_data' => '',
'unicity' => 1,
'plugin_flyvemdm_policycategories_id' => $category,
'comment' => __('Use TLS.', 'flyvemdm'),
'comment' => __('Use TLS', 'flyvemdm').'.',
Copy link
Contributor

Choose a reason for hiding this comment

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

put the dot in the localizable string

Copy link
Contributor Author

@DIOHz0r DIOHz0r May 24, 2018

Choose a reason for hiding this comment

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

Why? the dot is at the end and we could use the same translation of the upper line or do you really want to have to translate two string just for only one dot?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in some languages the punctuation may be very different to latin languages. We cannot do assumption about locales.

Moreover this policy willl change its name see #494 We wil need to improve comment and probably use longer text than only copying the name.

}
if ($DB->fieldExists($table, 'is_apple_policy')) {
$migration->dropField($table, 'is_apple_policy');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Migration::dropField() already checks if the field exists before actually dropping it. This change is useless

@ajsb85 ajsb85 self-requested a review May 24, 2018 11:02
@ajsb85 ajsb85 added the bug label May 24, 2018
@ajsb85 ajsb85 added this to the 2.0 milestone May 24, 2018
@ajsb85 ajsb85 changed the title Patch for PR of OS min/max versions WIP Patch for PR of OS min/max versions May 24, 2018
@DIOHz0r DIOHz0r force-pushed the hotfix/os_min_max_versions branch from 0bfc786 to b719ae0 Compare May 24, 2018 12:52
@ajsb85
Copy link
Contributor

ajsb85 commented May 24, 2018

Hi, @DIOHz0r
fix(install): fix for db schema update and locale
Should be:
fix(install): db schema update and locale
Thanks

@DIOHz0r DIOHz0r force-pushed the hotfix/os_min_max_versions branch from b719ae0 to 10fd9bb Compare May 24, 2018 14:51
@Naylin15
Copy link
Contributor

Fixing issues that complement this PR and making several tests in the server

I think I found the solution but I need to test it some more

One of the main issues was that the DB was not being updated properly, the values of the os versions were not being added when running cli_install.php

@btry
Copy link
Contributor

btry commented May 25, 2018

Indeed. I'll point to you the code that you need to update.

@btry
Copy link
Contributor

btry commented May 25, 2018

@Naylin15 have a look into the PluginFlyvemdmInstaller::createPolicies()

https://github.com/flyve-mdm/glpi-plugin/blob/develop/install/installer.class.php#L323

This method is in charge of creating / updating the policies in the database. You need to modify it to take into account the new columns you introduced. Once done, the update process should run as you expected.

@DIOHz0r DIOHz0r changed the title WIP Patch for PR of OS min/max versions Patch for PR of OS min/max versions May 25, 2018
@@ -43,7 +43,7 @@
'type_data' => '',
'unicity' => 1,
'plugin_flyvemdm_policycategories_id' => $category,
'comment' => __('Use TLS.', 'flyvemdm'),
'comment' => __('Use TLS', 'flyvemdm').'.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in some languages the punctuation may be very different to latin languages. We cannot do assumption about locales.

Moreover this policy willl change its name see #494 We wil need to improve comment and probably use longer text than only copying the name.

DIOHz0r and others added 3 commits May 25, 2018 10:47
@DIOHz0r DIOHz0r force-pushed the hotfix/os_min_max_versions branch from 4064bd8 to 4d732cf Compare May 25, 2018 14:47
Copy link
Contributor

@btry btry left a comment

Choose a reason for hiding this comment

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

LGTM

@ajsb85 ajsb85 merged commit ccdff7f into flyve-mdm:develop May 25, 2018
@DIOHz0r DIOHz0r deleted the hotfix/os_min_max_versions branch May 25, 2018 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants