CRM-20981 - Allow custom base-pages with less crmApp boilerplate

[totten]

Overview

This provides API improvements for extensions (such as CiviCase and CiviVolunteer) which implement custom Angular base-pages.

Before

The CiviCase extension created its own standalone base-page with this:

  public function run() {
    $loader = new \Civi\Angular\AngularLoader();
    $loader->setPageName('civicrm/case/a');
    $loader->setModules(array('crmApp', 'civicase'));
    $loader->load();
    \Civi::resources()->addSetting(array(
      'crmApp' => array(
        'defaultRoute' => '/case/list',
      ),
    ));
    return parent::run();
  }
  /**
   * @inheritdoc
   */
  public function getTemplateFileName() {
    return 'Civi/Angular/Page/Main.tpl';
  }

After

The CiviCase extension can create the same page with this:

  public function run() {
    $loader = new \Civi\Angular\AngularLoader();
    $loader->setPageName('civicrm/case/a');
    $loader->setModules(array('civicase'));
    $loader->useApp(array(
      'defaultRoute' => '/case/list',
    ));
    $loader->load();
    return parent::run();
  }

Comments

• For backwards compatibility, the useApp() function is opt-in. This means that the old code in the CiviCase extension (for both civicrm/case/a and for "View Contacts") still works the same.
• After merging, we should update the dev docs in loader.md.

---

• CRM-20981: Allow custom base-pages with less crmApp boilerplate

[totten]

Ping @GinkgoFJG

[seancolsen]

I haven't reviewed this PR, but I'd will see if I can find time in my schedule to do it within the next week. But in the mean time I'll offer this feedback:

I'd like to see documentation added before merging this PR.

[totten]

Well, I'd say the goal is to kill-off a few lines of documentation. :) But you can see what I mean in ^^.

[universalhandle]

I'm out of town for another week but will gladly review this when I get back. On Aug 4, 2017 3:12 PM, "Tim Otten" <notifications@github.com> wrote: Well, I'd say the goal is to kill-off a few lines of documentation. :) But you can see what I mean in ^^. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10783 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1NixtIIWmNBObmdqslLYKIDBvE2RM1ks5sU5djgaJpZM4OnMpc> .

[colemanw]

@GinkgoFJG that would be very helpful thanks.

[seancolsen]

In this documentation for $settings I think it would be helpful to have some example of values which can be supplied for activeRoute, defaultRoute, and region. This would help me understand whether I should doing things like using a preceding forward slash or not.

[seancolsen]

Note: I have requested some changes to the corresponding Dev Guide PR here: civicrm/civicrm-dev-docs#244

I think it would be good to resolve those requests before merging this PR.

[seancolsen]

Here's another issue I had while reviewing this PR locally...

With my custom base page, http://pr/civicrm/foo/#/foo, the crm-page-title AngularJS directive is not correctly changing the <title> element or the main <h1> element.

Whereas with the default base page http://pr/civicrm/a/#/foo, the crm-page-title AngularJS directive works as it should:

Troubleshooting info:

• In my browser console on the custom base page, I tried this:

  >> CRM.angular.modules
     Array [ "Foo", "crmApp", "crmResource", "crmUi", "crmUtil", "ngRoute" ]
  

[seancolsen]

I have reviewed this PR locally on the following criteria and I think it may need some changes before being merged.

• ✅ It's a worthy pursuit.
• ❌ It works as advertised.
  • I was able to get a custom base page working.
  • But the custom base page behaves slightly differently than the default base page, as noted in my comment above.
  • I didn't go too deep into comparing AngularJS pages with the default base page vs the custom base page. Perhaps there are other differences. I didn't put in enough time to be able to understand the root cause of this difference. Tim, if you can figure out why the pages are different then I think a suitable course of action would be either: (a) update the code in this PR to resolve this discrepancy, or (b) update the documentation to explain the discrepancy (and potentially provide workarounds).
• ✅ It makes sense for all users who may be affected.
• ✅ It's unlikely to confuse users.
• ✅ It doesn't appear to cause bugs or unwanted side effects.
• ✅ It doesn't appear to introduce security vulnerabilities.
• ✅ New code is either: (a) not present, or (b) readable and sensible.
• ✅ Acceptance criteria are either: (a) not stated, or (b) met.
• ✅ Significant new functionality is either: (a) not present, or (b) covered by new unit tests.
• ❌ New documentation is either: (a) not needed, or (b) provided within this PR, or (c) provided in a separate PR which already exists.
  • Overall, good! But...
  • Minor change requested to code-level documentation, as noted above
  • Minor change requested to corresponding Dev Guide PR

Ping @totten

[universalhandle]

Sorry to weigh in late on this.

I know I initially complained about having to manually add crmApp (which 99.999% of extensions will do because most people aren't going to be harnessing Angular on their own) and about having to set the route via a different class, but, honestly, I don't think anyone is going to be able to create their own Angular base page without reading some documentation. And the documentation is pretty clear about what the process is (though perhaps could use some detail on what crmApp is and when/why you might need it).

Perhaps the syntax is sugary enough already and we should instead focus on improving the documentation, or on creating a civix helper for base pages. I reviewed (but have not executed) the code, and it makes sense to me, but it appears a few things aren't working for @seanmadsen.

If folks really want to see these changes merged in, I can try to find some time (not before next week, unfortunately) to apply this patch to a Volunteer sandbox (and make the corresponding pages to the extension) and see if I can reproduce/resolve the issues Sean reported. Let me know.

[seancolsen]

I chatted with Tim in-person about this PR at the sprint. He pointed out that I was comparing the wrong things in my review. I compared "custom base page with PR" to "standard base page with PR", when I should have compared "custom base page with PR" to "custom base page without PR".

I have re-done that portion of my review by looking at "custom base page without PR" which looks like this:

So basically, the page title issue that I noticed in my first review is unrelated to this PR.

The documentation issues are still valid as far as I'm concerned.

[eileenmcnaughton]

@totten @GinkgoFJG @seanmadsen after looking at the discussion above I'm inclined to merge this. It seems stalled on documentation, which is important but given the age of this PR I think we need to either merge or abandon at this stage & it seems mergeable

[totten]

Tend to agree @eileenmcnaughton. I almost merged it a few weeks ago under the same rationale but didn't quite feel I (as original author) should waive that critique -- it's valid but small.

For the main dev docs, there's a corresponding PR already (civicrm/civicrm-dev-docs#244). IIRC, the outstanding point was really about adding some more docblock examples, which I've now pushed up.

[universalhandle]

Works for me if @seanmadsen's happy.

[eileenmcnaughton]

OK - let's give @seanmadsen a couple of days to scream blue murder & then the next merger to look at this should merge (ie. @totten that's blessing to self-merge this from me if no objection)

[seancolsen]

Yep. @eileenmcnaughton I think this is ready to merge. All of my review critiques have been addressed. 😁

When you merge, can you either: merge this corresponding docs PR civicrm/civicrm-dev-docs#244 ; or ping me so I can merge it?

[eileenmcnaughton]

@colemanw you set the version in JIRA to 5.0.0 - should be 5.1.0 as rc cut - just noting because you are likely to be doing that for others too