Problem/Motivation

At the moment we have only one route that is not related to dashboard in UI module. Move that to crm_core so we can rename ui module to crm_core_dashboard

Proposed resolution

Move crm_core.manage route and it's menu link from crm_core_ui to crm_core.

CommentFileSizeAuthor
#18 move_non_dashboard-2703637-18.patch7.06 KBthenchev
#18 interdiff-2703637-18.txt445 bytesthenchev
#15 move_non_dashboard-2703637-15.patch7.07 KBthenchev
#15 interdiff-2703637-15.txt2.29 KBthenchev
#13 move_non_dashboard-2703637-13.patch5.04 KBthenchev
#11 move_non_dashboard-2703637-11.patch5.05 KBthenchev
#11 interdiff-2703637-11.txt1.48 KBthenchev
#9 interdiff-2703637-9.txt2.97 KBthenchev
#9 move_non_dashboard-2703637-9.patch3.56 KBthenchev
#3 move_non_dashboard-2703637-3.patch2.02 KBthenchev
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Assigned: Unassigned » thenchev
thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Active » Needs review
FileSize
move_non_dashboard-2703637-3.patch2.02 KB

Moved the route and menu link.

CTaPByK’s picture

CTaPByK CreditAttribution: CTaPByK at MD Systems GmbH commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks ok. RTBC.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Reviewed & tested by the community » Needs work

Not quite.

If the goal is to make crm_core_ui an optional dashboard module, then we need to ensure that that's actually the case with this issue (it becoming optional)

For that, we should update our dependencies to no longer depend on it and also no longer enable it in test.

I'm expecting at least some test fails because we don't take over the /crm-core route. If nothing else, then I'm expecting some breadcrumb test fails. Actually more than that, crm_core_contact and activity will both fail hard without that route.

This might have to wait until we know what we will happen with #2703641: Port crm_core_ui_dashboard() (not actually implemented, just defined).

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented

I discussed this with @berdir a bit. Let's also move "crm_core.dashboard" route too. This will prevent major problems, but we will need to create custom access check for it. We will do that in the follow-up.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented

Also, rename the route as it is not dashboard any more. "crm_core.overview" or something along those lines.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented

Follow-up: #2704095: Create custom access check for /crm-core

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
move_non_dashboard-2703637-9.patch3.56 KB
interdiff-2703637-9.txt2.97 KB

Moved crm_core.dashboard and renamed it. Removed all dependencies i could find.

Status: Needs review » Needs work

The last submitted patch, 9: move_non_dashboard-2703637-9.patch, failed testing.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
interdiff-2703637-11.txt1.48 KB
move_non_dashboard-2703637-11.patch5.05 KB

This should fix the test fails. Created permissions.yml in crm core for the overview route. I guess we can later change that in the followup...

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented

Looks good.

+++ b/crm_core.links.menu.yml
@@ -0,0 +1,11 @@
+  description: 'Manage contact types, activity types, and contact relationship types..'

One dot extra at the end. I see it was the same before though...

I guess we already have some tests for those menu links/routes?

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
FileSize
move_non_dashboard-2703637-13.patch5.04 KB

Neutralized the dot and also a comma.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Test is a good point, I can't see any tests that directly access /crm-core.

Lets add that to some existing contact tests and then click on the menu link that should be there instead of going to crm-core/contacts directly. We will also need to give the user we do it with the new permission.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
interdiff-2703637-15.txt2.29 KB
move_non_dashboard-2703637-15.patch7.07 KB

Expanded test coverage, fixed menu links for contact and activity.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks, good thing I requested test coverage then ;)

Not sure if the links really need to contain CRM there, seems repetitive. But that's a different issue and not very important.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Reviewed & tested by the community » Needs work 
+++ b/modules/crm_core_contact/crm_core_contact.info.yml
@@ -8,9 +8,6 @@ core: 8.x
-  # Core dependencies
-  - crm_core_ui
-

Before we removed this this implicitly added dependency to crm_core. That is not the case any more. It seems that all submodules need to explicitly depend on crm_core.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
interdiff-2703637-18.txt445 bytes
move_non_dashboard-2703637-18.patch7.06 KB

Updated dependency for crm_core_contact since it seems that all the other submodules are directly or indirectly dependent on crm_core

CTaPByK’s picture

CTaPByK CreditAttribution: CTaPByK at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Dependency is added so i think we can back status to RTBC.

  • slashrsm committed a56e49e on 8.x-1.x authored by Denchev 
    Issue #2703637 by Denchev, slashrsm, Berdir, CTaPByK, mbovan: Move non-...
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.