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

Multiple website support #274

Merged
merged 71 commits into from
Jun 1, 2021
Merged

Multiple website support #274

merged 71 commits into from
Jun 1, 2021

Conversation

boehsermoe
Copy link
Member

@boehsermoe boehsermoe commented Jul 15, 2020

What are you changing/introducing

Handle multiple websites/domain in one luya cms module. Nav container will be assign to a website and all navs filter by website.

Screenshot 2020-07-15 at 20 51 18Screenshot 2020-07-15 at 20 51 57

Todo's

  • Backward compatible
  • Docu
  • CMS Pages
  • Nav

QA

Q A
New feature? yes
Breaks BC? yes
Tests pass? yes
Fixed issues #252

@nadar
Copy link
Member

nadar commented Jul 16, 2020

As its not ready for review i just want to ask conceptional questions: do i need to create a website first? is this backwards compatible?

Looking forward! thats a great feature, thank you!

@JohnnyMcWeed
Copy link
Contributor

What does this mean in terms of custom modules?

@boehsermoe
Copy link
Member Author

boehsermoe commented Jul 16, 2020

do i need to create a website first? is this backwards compatible?

yes a one website is always needed but in the migration a default website will be created.
But I can try to work without any website, then it should be backwards compatible but I have to take a closer at that.

@JohnnyMcWeed

What does this mean in terms of custom modules?

What exactly do u mean? Every module and controller would available on each website. In the module you can ask for the current website.
e.g. for news module it would mean that can can share the same news and categories for all websites. The news module could need also an update to assign news and categories to single websites.

@JohnnyMcWeed
Copy link
Contributor

What exactly do u mean? Every module and controller would available on each website. In the module you can ask for the current website.
e.g. for news module it would mean that can can share the same news and categories for all websites. The news module could need also an update to assign news and categories to single websites.

Correct :) So the docs should then take this into account aswell.
Maybe it's possible to make some kind of interface for module blocks to use it directly?
E. g. add one column to a table, use the model interface and it's connected by default... (just a quick thought about it...)

@boehsermoe
Copy link
Member Author

So the docs should then take this into account aswell.

Yes, of course the docu still has to be adapted.

Maybe it's possible to make some kind of interface for module blocks to use it directly?
E. g. add one column to a table, use the model interface and it's connected by default... (just a quick thought about it...)

That a good idea! Maybe doing it with a behaviour or a trait. And also a trait for model filtering like SoftDeleteTrait can be useful.

I think I will start also a new branch in the news module and take a closer look.

@nadar
Copy link
Member

nadar commented Jul 17, 2020

@boehsermoe i think full backwards compatibility is a must. A website in the migration is good for me, but if only one (1) website is defined we should hide "change navigation for domain" button. Because 99% of the cases is what we have now and not the opposite i would say.

Also when i was thinking of how the solve this issue with multiple domains, keep in mind we do have already definitions for this information: https://github.com/luyadev/luya/blob/master/core/web/Composition.php#L88-L123

Thanks for the work.

Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

The migration worked perfect, i could NOT detected any BC break! 👍

  • after saving or updating the websites you need the refresh the websites services, otherwise a browser reload is required. which is not nice. Here an example from the docs
public function ngRestConfigOptions()
{
    return [
        'saveCallback' => "['ServiceMenuData', function(ServiceMenuData) { ServiceMenuData.load(true); }]",
    ];
}
  • After i have created a new website, now i have two default containers when i create a new page, this is really a big problem. Either we have to prefix the website in the container names or the website must be choosen first:

image

  • When i switch to new container (which does not have sites yet, as i just have created that website), there is a js error

image

@boehsermoe
Copy link
Member Author

When i switch to new container (which does not have sites yet, as i just have created that website)

I could not reproduce the js error. Where exactly it happens?

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5251 lines exceeds the maximum allowed for the inline comments feature.

@@ -181,8 +186,12 @@
$scope.data.parent_nav_id = 0;
$scope.data.is_draft = 0;

$scope.data.nav_container_id = 1;

$scope.data.nav_container_id = ServiceCurrentWebsite.currentWebsite.default_container_id;
Copy link
Member

@nadar nadar May 27, 2021

Choose a reason for hiding this comment

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

This will throw an exception (and breaks the whole create page form) unless the object is resolved.

If event service:CurrentWebsiteChanged is broadcasted anyway, then this line is not required. But i am not sure about that

The problem is that $scope.data.nav_container_id = ServiceCurrentWebsite.currentWebsite.default_container_id; will throw an type error Cannot read property 'default_container_id' of undefined because the state of service.currentWebsite object is by default

var service = {
		currentWebsite: null,
}

@nadar
Copy link
Member

nadar commented May 27, 2021

When i switch to new container (which does not have sites yet, as i just have created that website)

I could not reproduce the js error. Where exactly it happens?

Indeed the problem has nothing to do with your changes. i will fix that after merge. BUT what i saw is that the create page sub container system does not work:

2021-05-27_09-50

The subpage system uses the data from the the menu, so if you are on another "website" the sub pages array is wrong. Therefore we could either see that when creating a new page, the context from the menu panel is required. Which means:

if you like to create a PAGE for domain foobar.com -> you have to select that domain on the left first.
This would remove the option to select all containers, and just show the containers from that domain/website.

What do you think?

@boehsermoe
Copy link
Member Author

Now both fields will use from the left menu.

@boehsermoe boehsermoe requested a review from nadar May 28, 2021 10:08
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 6438 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 6438 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

codeclimate bot commented Jun 1, 2021

Code Climate has analyzed commit 9364065 and detected 18 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 14
Duplication 4

The test coverage on the diff in this pull request is 67.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 50.2%.

View more on Code Climate.

@nadar nadar merged commit 9393e34 into luyadev:master Jun 1, 2021
@bastelix
Copy link

seems this cann be added here. There is an generell issue when adding a new container for a website. e.g:

I have some different websites:
SNAG-0688

and when I add a new container for theming:
SNAG-0689

it results alltimes in using first created website:
SNAG-0690

seems an issue in cms module for multi website support

@nadar
Copy link
Member

nadar commented Dec 13, 2023

@bastelix please create a fresh issue. I have tested to save and update the website for containers on the latest version and it works as expected.

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.

None yet

4 participants