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

[ENHANCE] - Added Module to NameSpace [Owner].Module.[Module] #2910

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

vnetonline
Copy link
Contributor

[ENHANCE] - Added Module to NameSpace [Owner].Module.[Module]

@sbwalker sbwalker merged commit 43bcfb9 into oqtane:dev Jun 21, 2023
@sbwalker
Copy link
Member

@vnetonline there appears to be an issue with this PR:

In Oqtane.Client\Modules\Admin\ModuleCreator\Index.razor it has:

    private void GetLocation()
    {
        _location = string.Empty;
        if (_owner != "" && _module != "" && _template != "-")
        {
            var template = _templates.FirstOrDefault(item => item.Name == _template);
            _location = template.Location + _owner + ".Module." + _module;

        }
        StateHasChanged();
    }

And in Oqtane.Client\Modules\Admin\ModuleDefinitions\Create.razor it has:

    private void GetLocation()
    {
        _location = string.Empty;
        if (_owner != "" && _module != "" && _template != "-")
        {
            var template = _templates.FirstOrDefault(item => item.Name == _template);
            _location = template.Location + _owner + "." + _module;

        }
        StateHasChanged();
    }

Notice the ".Module." identifier in the _location in the first one which does not exists in the second one.

@sbwalker
Copy link
Member

sbwalker commented Aug 22, 2023

@vnetonline after further research... this PR impacts the ability for people to create their own custom templates. The template feature was developed with extensibility in mind... you can include your own custom templates beneath the Oqtane.Server\wwwroot\Modules\Templates folder and the UI will automatically recognize them and allow you to choose them from the list. However the problem is that there are some properties which are set independently from the template - they are set in code:

                    moduleDefinition.ModuleDefinitionName = moduleDefinition.Owner + ".Module." + moduleDefinition.Name + ", " + moduleDefinition.Owner + ".Module." + moduleDefinition.Name + ".Client.Oqtane";
                    moduleDefinition.ServerManagerType = moduleDefinition.Owner + ".Module." + moduleDefinition.Name + ".Manager." + moduleDefinition.Name + "Manager, " + moduleDefinition.Owner + ".Module." + moduleDefinition.Name + ".Server.Oqtane";

So if you were using a custom module template, this PR now forces a ".Module." segment in your namespaces which would be incompatible with your custom template. @leigh-pointer has some custom module templates which are available for download which likely now have issues due to this change. I believe that the optimal solution is for a template to be able to describe their behavior so that the framework does not make incorrect assumptions or force a specific approach. Perhaps these properties could be added to the Template.json file to mitgate this issue.

This is my fault for not doing a thorough impact assessment. This item will require more research.

@vnetonline
Copy link
Contributor Author

vnetonline commented Aug 22, 2023

@sbwalker I see your point, on a further thought about this, wouldn't it be better if developers were able to specify their namespace as a whole?

For example, a developer could have various related modules in a simple case:

Acme.Bookstore which has

  • The owner Acme and product Bookstore

A developer could also have a more complex case like:

Acme.Software.Bookstore which has

  • The owner Acme,
  • The product Software
  • The module in that software Bookstore

Further, they could have functionality within their module like Acme.Software.Bookstore.Payments

  • The owner Acme,
  • The product Software
  • The module in that software Bookstore
  • The functionality in the Bookstore Payments or Cart etc

we could then parse the Namesapce to take the last element (nested Namespace) to create the ServerManagers or Repositores or Controllers or Services... etc like

  • PaymentManager and CartManager
  • PaymentRespository and CartRepository
  • PaymentService and CartService
  • PaymentController and CartController

Also, the template creator doesn't have plurals in mind for example if I have the following
Amazing.Module.Audience

The table name and Get List methods should be called Audiences which is plural of the module called Audience, however, the Entity should be Audience as it defines one Audience

So having fields for just Owner and Product in the UI restrict this ability to be truly extensible for the developer we could just have one field to fill in which is their NameSpace.

This would remove the .Module. and allow the developer to have a lot more flexibility, with just some convention that would conform to some sort of consistency.

open to your thoughts!

@sbwalker
Copy link
Member

@vnetonline after reviewing this further, there may not be any need for the logic which sets the ModuleDefinitionName or ServerManagerType... these are only required for the Module Creator scenario (ie. where someone adds a Module Creator modue to a page). There is talk in a different thread about removing that option and only relying on the Create Module option in Module Management (which does not require the logic above).

In regards to the suggestion about asking the user for a Namespace, why couldn't a developer simply enter "Acme.Software.Bookstore" as their Organization? I believe the templating logic supported this previously. In fact, if you wanted to use the convention which includes ".Module" you could have entered it into the Organization field as "MyCompany.Module". But after the changes in this PR it actually forces the ".Module" segment whether you want it or not. I am wondering if this needs to be rolled back.

@vnetonline
Copy link
Contributor Author

Yup I agree we should roll this back

@vnetonline
Copy link
Contributor Author

Let me know if you want me to submit a PR

@sbwalker
Copy link
Member

sbwalker commented Aug 28, 2023

@vnetonline #3200 introduces a Namespace property to the template.json which can be used to define the naming convention in a template. It eliminates the hard-coding and restores freedom to custom templates. We can leave the default templates in their current form... people can create custom templates if they want a different naming convention.

@vnetonline
Copy link
Contributor Author

Thanks @sbwalker i will give it a test and report back

@sbwalker
Copy link
Member

sbwalker commented Sep 1, 2023

@vnetonline I am guessing you did not have a chance to test this. It appears that my change caused a problem with ServerManagerType which I will need to fix in 4.0.4.

@vnetonline
Copy link
Contributor Author

Sorry @sbwalker didn't get chance to to test this I will find some time today

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.

2 participants