new feat: Custom company name #26
Conversation
📝 WalkthroughWalkthroughThis PR introduces company name support throughout the SwitchCraft application, adding policy definitions for enforced and default configurations, localized UI strings in English and German, a configuration accessor method, a settings view input field, and enhanced template generation with company metadata in file headers. Test coverage is provided for the new template functionality. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsUI as Settings View
participant Config as SwitchCraftConfig
participant TemplateGen as TemplateGenerator
participant Template as Intune Template
User->>SettingsUI: Enter Company Name
SettingsUI->>Config: set_user_preference("CompanyName", value)
Config->>Config: Save to storage
User->>TemplateGen: Generate template
TemplateGen->>Config: get_company_name()
Config-->>TemplateGen: Return company name
TemplateGen->>TemplateGen: Build context with COMPANY_NAME
TemplateGen->>TemplateGen: Build HEADER_CREATED_BY
TemplateGen->>Template: Inject {{HEADER_CREATED_BY}}
Template-->>User: Template with company header
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/PolicyDefinitions/SwitchCraft.admx (2)
115-121: Consider making CompanyName optional.Line 119 sets
required="true"for the CompanyName field. This might be too restrictive, as not all users work for a company or may want to omit this field. Consider changing torequired="false"to make it optional, especially since the code already handles empty company names gracefully (Line 78-81 in templates.py checks for empty values).🔎 Proposed change
- <text id="CompanyNameBox" valueName="CompanyName" required="true" /> + <text id="CompanyNameBox" valueName="CompanyName" required="false" />
222-228: Consider making CompanyName optional.Similar to the enforced policy, Line 226 sets
required="true"for the default CompanyName policy. This should also be reconsidered to allow optional configuration.🔎 Proposed change
- <text id="CompanyNameBox" valueName="CompanyName" required="false" /> + <text id="CompanyNameBox" valueName="CompanyName" required="false" />tests/test_template_company.py (2)
14-37: Consider mockingSwitchCraftConfig.get_valuefor isolation.The
TemplateGenerator.__init__callsSwitchCraftConfig.get_value("CustomTemplatePath")which is not mocked. This could cause the test to depend on actual configuration state, reducing test isolation and potentially causing intermittent failures.🔎 Proposed fix to add config mock
+ @patch("switchcraft.utils.config.SwitchCraftConfig.get_value") @patch("switchcraft.utils.config.SwitchCraftConfig.get_company_name") @patch("os.environ.get") - def test_generate_with_company_name(self, mock_env, mock_get_company): + def test_generate_with_company_name(self, mock_env, mock_get_company, mock_get_value): + mock_get_value.return_value = None # No custom template path mock_get_company.return_value = "Acme Corp" mock_env.return_value = "TestUser"
39-54: Consider mockingSwitchCraftConfig.get_valuefor isolation.Same as the first test,
TemplateGenerator.__init__callsSwitchCraftConfig.get_value("CustomTemplatePath")which should be mocked for test isolation.🔎 Proposed fix to add config mock
+ @patch("switchcraft.utils.config.SwitchCraftConfig.get_value") @patch("switchcraft.utils.config.SwitchCraftConfig.get_company_name") @patch("os.environ.get") - def test_generate_without_company_name(self, mock_env, mock_get_company): + def test_generate_without_company_name(self, mock_env, mock_get_company, mock_get_value): + mock_get_value.return_value = None # No custom template path mock_get_company.return_value = "" # No company mock_env.return_value = "TestUser"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/PolicyDefinitions/SwitchCraft.admxdocs/PolicyDefinitions/de-DE/SwitchCraft.admldocs/PolicyDefinitions/en-US/SwitchCraft.admlsrc/switchcraft/assets/lang/de.jsonsrc/switchcraft/assets/lang/en.jsonsrc/switchcraft/assets/templates/DefaultIntuneTemplate.ps1src/switchcraft/gui/views/settings_view.pysrc/switchcraft/utils/config.pysrc/switchcraft/utils/templates.pytests/test_template_company.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_template_company.py (1)
src/switchcraft/utils/templates.py (2)
TemplateGenerator(8-143)generate(40-143)
src/switchcraft/gui/views/settings_view.py (2)
src/switchcraft/utils/i18n.py (1)
get(116-143)src/switchcraft/utils/config.py (2)
get_company_name(24-26)set_user_preference(134-175)
src/switchcraft/utils/templates.py (1)
src/switchcraft/utils/config.py (1)
get_company_name(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Installer (windows-latest)
🔇 Additional comments (13)
src/switchcraft/assets/lang/en.json (1)
259-259: LGTM!The localization key follows the existing naming convention and integrates seamlessly with other settings keys.
docs/PolicyDefinitions/de-DE/SwitchCraft.adml (1)
65-118: LGTM!The German policy resources are well-structured and follow the established pattern for other policy definitions. The translations are appropriate and the presentation markup is correct.
docs/PolicyDefinitions/en-US/SwitchCraft.adml (1)
65-118: LGTM!The English policy resources are properly structured and follow the same pattern as other policy definitions. The help text clearly explains the purpose of the setting.
src/switchcraft/assets/lang/de.json (1)
280-280: LGTM!The German localization is correctly added and matches the English counterpart.
src/switchcraft/utils/config.py (1)
23-26: LGTM!The accessor method follows the established pattern in this class and correctly returns an empty string as the default value.
src/switchcraft/gui/views/settings_view.py (1)
58-72: LGTM!The Company Name UI implementation follows the existing patterns in the settings view. The save-on-blur and save-on-enter behavior is consistent with good UX practices.
src/switchcraft/utils/templates.py (1)
65-82: LGTM with a minor observation!The template context enrichment is well-implemented. The conditional header generation appropriately handles cases with and without a company name.
One minor note: Line 78 checks for
company != "Unknown Company". This specific string doesn't appear to be defined elsewhere in the codebase as a constant or default value. If this is a possible value that might be set elsewhere, consider defining it as a constant for consistency. If not, this check may be unnecessary sinceget_company_name()returns an empty string by default.tests/test_template_company.py (2)
1-12: LGTM!The test class structure, imports, and cleanup logic are well-organized and follow standard unittest patterns.
56-57: LGTM!Standard unittest entry point.
src/switchcraft/assets/templates/DefaultIntuneTemplate.ps1 (4)
3-3: LGTM!The
{{HEADER_CREATED_BY}}placeholder correctly integrates with the template generation logic insrc/switchcraft/utils/templates.py, which conditionally includes company name metadata.
31-34: LGTM!Helpful documentation clarifying the log redirection strategy for MSI/EXE installers.
68-70: LGTM!Logging instead of attempting to show a toast notification under SYSTEM context is the appropriate behavior.
157-160: LGTM!Enhanced error handling with separate exception message and stack trace logging provides better debugging information.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.