This processor wasn't really ported at all yet, so it both needs a working implementation and tests.

The UI should have the same look and feel like in D7 (AJAX), see the screenshots (order from bottom to top)

  1. The tab with a button to add a new aggregated field
  2. Add the field, choosing existing fields and the aggregation type
  3. The aded field, possible to delete (and maybe new functionality: update)
  4. And always, the possibility to add a new one

Comments

ytsurk’s picture

ytsurk’s picture

Issue summary: View changes
ytsurk’s picture

Issue summary: View changes
ytsurk’s picture

Issue summary: View changes
drunken monkey’s picture

Why no test ?

"no test" probably means that there is none, I'm pretty sure it should get one.

Also, a new type for creating just a verbatim copy of one or more fields would be very helpful, I think – and since we don't distinguish between single- and multi-valued any more, also pretty easy. (This change might also make most other functionality of this processor much less useful.)

ytsurk’s picture

(This change might also make most other functionality of this processor much less useful.)

so - this processor can be dropped ? or is there still a need ?
somehow it was already process code written, but that is now broken :(

drunken monkey’s picture

No, I think we should keep it. "Much less" was probably exaggerated. But we should probably make "Copy verbatim" (or whatever the label) the default option, once we've added it.

drunken monkey’s picture

Title: Implement 'Add Aggregation' Settings Form » Port "Aggregated fields" processor
Issue summary: View changes

In the course of #2252079: Switch to proper objects for extracted items I noticed that this is currently not really ported at all. More or less only the method names were changed, but the implementation is still the same as in D7 (which of course won't work anymore).

@ ytsurk: Are you still working on this? If so, just ask any questions you might have (here or in IRC), I'd love to assist you in implementing this.

nick_vh’s picture

Status: Active » Needs work

Ported and confirmed as working. Pending is to write tests for it. Committed to the D8 master branch of the sandbox.

Some important differences. The add_aggregated processor was renamed to aggregated_field processor to more accurately reflect what it does. The whole form ajax was updated to D8 standard and uses static function to return the form element it needed.
Also it now checks if the field that the site builder wants to aggregate is from the same datasource type as the item that was given to the processor. If not, it will ignore it.

It was manually tested to see if the data reflected well in the database backend.

nick_vh’s picture

Pending:
Write tests for all cases
Add the notification that it won't be saved until the user saves the processor page.

drunken monkey’s picture

Assigned: ytsurk » drunken monkey

Already added #2305021: Test field extraction in processors as a follow-up for the slightly more complicated (and hopefully less critical) task of testing the field extraction for unindexed fields that are aggregated.

Already worked quite a bit on the rest, should be ready to commit soon.

nick_vh’s picture

Started to work on a test for the aggregated fields processor. Bunch of schema validation processor errors were corrected first as the validation was committed to Drupal 8.

drunken monkey’s picture

Status: Needs work » Fixed

Sorry, but we probably won't have use for that – as written above (and marked by the "Assigned" change), I've been working on this, and now I already have a working processor, the schema fixes and a decent test class.
Of course, if you have additional fixes or tests, please merge them in.

Anyways, the tests all pass for me, except the Views test which now outright fails with an exception. No idea what that's about, but will look into it if I can.

  • drunken monkey committed 9733a4d on master
    Issue #2256943 by drunken monkey: Fixed "Aggregated fields" processor...
drunken monkey’s picture

Assigned: drunken monkey » Unassigned
Status: Fixed » Active

Oh, and one additional thing I'm not sure about, is whether we shouldn't rename the class (along with its test class, of course) to AggregatedFields (with appended "s") to reflect its label. After all, it doesn't just provide a single field. Any comments on that?

  • drunken monkey committed 65b493e on master
    Follow-up to #2256943 by drunken monkey: Removed now unneeded reduce()...

  • drunken monkey committed f14410f on master
    Follow-up to #2256943 by drunken monkey: AggregatedField::flattenArray...

  • Nick_vh committed 143a011 on master
    Issue #2256943 by Nick_vh: Fix config schemas for aggregated fields...

  • Nick_vh committed 143a011 on 2286813-fields-processor-plugin-base-test
    Issue #2256943 by Nick_vh: Fix config schemas for aggregated fields...
  • drunken monkey committed 65b493e on 2286813-fields-processor-plugin-base-test
    Follow-up to #2256943 by drunken monkey: Removed now unneeded reduce()...
  • Nick_vh committed 9380eb1 on 2286813-fields-processor-plugin-base-test
    Issue #2256943 by Nick_vh: Added a newline
    
  • drunken monkey committed 9733a4d on 2286813-fields-processor-plugin-base-test
    Issue #2256943 by drunken monkey: Fixed "Aggregated fields" processor...
  • drunken monkey committed f14410f on 2286813-fields-processor-plugin-base-test
    Follow-up to #2256943 by drunken monkey: AggregatedField::flattenArray...
drunken monkey’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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