Problem/Motivation

The javascript api descriped in https://www.drupal.org/node/336122 has not been portet to the 8.x version of the flags module.

Proposed resolution

Refactor the 7.x api for drupal 8.x

User interface changes

None

API changes

Add API like in 7.x

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Laubi created an issue. See original summary.

joachim’s picture

Title: [Regression] Restore javascript api » [Regression] Restore Javascript APi
Issue tags: +beta blocker

Thanks for reporting!

martin107’s picture

To my mind

inserting before and after pseudo events, is a good idea

I am postponing until we have a ajax command associated with ajax flag actions.

In the issue I have linked to, its current name is ActionLinkFlashCommand, but from this issue I can see that we need to be make that name more generic.

martin107’s picture

Status: Postponed » Active

I am postponing until we have a ajax command associated with ajax flag actions.

We have that command now.

martin107’s picture

Status: Active » Needs review
FileSize
7.21 KB

Here is the code that adds before and after events for third party modules to bind their event listeners to.
I have included a 'working' test modules that adds the appropriate event listeners.

If you copy "flag_flash_event_test" out of the test directory into the custom module directory then you will see it adds a flag which attaches to any article that you care to create .. clicking on the new flag will cause the third party module to respond to before and after events. It dumps messages onto the console.

The next steps are to write a functional javascript test.. but I am inserting a pause here to show my working out .. for people to criticise. --- When I am sure the architecture is ok I will write the test

Status: Needs review » Needs work

The last submitted patch, 5: outline-2842386-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

socketwench’s picture

Status: Needs work » Needs review

Test failures unrelated.

socketwench’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

...of a successful command respoonse

Small typo there.

Also needs tests, but I'm liking this.

martin107’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
12.03 KB
9.47 KB

From #8 "small typo" ... thanks .. can't think why I was thinking of spoons while i mistyped that!

1) I am trying to shadow the functionality provided by the drupal7 version of this module.. and I see after a little review that I had missed a little bit of piping. In this iteration the response object is now plumbed down into the customEvents

2) To nail thing down in testing I am vandalizing the page by adding two paragraphs elements one for each event. I have found a way of locking down the test so they fail if the response object is not included as a detail object in the event.

3) With regard to namespacing, others may extend ActionLinkFlashCommand so in a couple of places I have made the name of things more unique,

for example

flagBefore becomes flagFlashBefore.

Status: Needs review » Needs work

The last submitted patch, 9: 2842386-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review

Ok so the failures in #9 are caused by our test instability

#2945195: test instability ( round 2 )

For which we I think we now have a solution ....

https://www.drupal.org/project/flag/issues/2945195#comment-12530969

So here I am retesting.

socketwench’s picture

Retesting now that the above issue is in.

Status: Needs review » Needs work

The last submitted patch, 9: 2842386-9.patch, failed testing. View results

socketwench’s picture

Issue tags: +Needs reroll
martin107’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.97 KB

Status: Needs review » Needs work

The last submitted patch, 15: 2842386-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
10.87 KB
2.77 KB

I have not fixed the test ... yet

but there are enough pieces of the solution here to be worth talking about.

The problem when debugging is that the js files provided by the mock third party module is not on the page.

ie event_listener.js - I have no solution to that yet.

Here are the changes ...

a) flag_flash_event_test/config/install/flag.flag.event_test.yml was creating when I had a different test stratergy
it is no longer needed.

b) The code below cannot tolerate white spaces.

  $p_after = $this->assertSession()->waitForElementVisible('css', "p.after:contains('After:$flag_short_text')");

c) I have made flag_flash_event_test.info.yml follow the pattern set in flag_test_plugin.info.yml

Status: Needs review » Needs work

The last submitted patch, 17: 2842386-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Postponed

So when I described in #17 ...

"Why is the javascript added in reality, in manual testing, but consistently screws up in test?"

Well this core issue is similar but not exactly that ....

#1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

I am going to postpone this and work on that ...