Skip to content

Changed validator for OverrideInThemeDialog #326

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

Merged
merged 7 commits into from
Oct 23, 2020

Conversation

drpayyne
Copy link
Contributor

@drpayyne drpayyne commented Oct 21, 2020

Description

This PR changes the validator of OverrideInThemeDialog.

Fixed Issues

  1. Change validation for the overriding in theme dialog window #304: Change validation for the overriding in theme dialog window

Questions or comments

  • Similar to new plugin dialog needing a list of modules (TODO: future PR), we'll need to handle a validation for a list of editable themes as mentioned below in the comment. @VitaliyBoyko, how do we handle this as well? I'm thinking we create a new validation rule, which checks the EditableThemes and return a bool of the match.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Comment on lines -57 to -69
final List<String> allThemesList = ModuleIndex.getInstance(project).getEditableThemeNames();
if (!allThemesList.contains(theme)) {
final String errorMessage = validatorBundle
.message("validator.module.noSuchModule", theme);
JOptionPane.showMessageDialog(
null,
errorMessage,
errorTitle,
JOptionPane.ERROR_MESSAGE
);

return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't implemented yet.

@drpayyne
Copy link
Contributor Author

I noticed that wherever com.magento.idea.magento2plugin.ui.FilteredComboBox is used as a component in a dialog form, two steps occur:

  • The dropdown is populated with the options. Either a list of ModuleIndex.getInstance(project).getEditableThemeNames() (themes) or ModuleIndex.getInstance(project).getEditableModuleNames() (modules)
  • Before saving and during validation, the selected option is checked if it exists in the very same list it was used to populate with

The seemingly unnecessary validation (step 2) is done because the FilteredComboBox is an editable field. So it makes sense to validate the selected option. But what doesn't make sense to me, is to let a field even be editable in the first place, if the validation of the selected option is done against the original list it was populated with (not possible to enter any value other than the values in the original list).

My suggestion:-

I have added a commit to this PR (ref: 0f65e53) which changes the field to a javax.swing.JComboBox (non editable dropdown, already used for plugin type and plugin area in CreateAPluginDialog), and this comment (ref: #326 (comment)) becomes irrelevant, since a validation isn't required for that field at all.

This issue is present in multiple PRs of mine where the validation is done for a FilteredComboBox with module names, but since I'm new to this project, I'd like to check if this approach is acceptable before porting this fix to those PRs.

CC/ @VitaliyBoyko

@VitaliyBoyko
Copy link
Contributor

Hi @drpayyne

Dropdown is editable for a filtering purpose. For themes, it is not necessary (I agree here), but for the modules list, it is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants