Feature/liveblog #261

tjwelde · 2017-03-07T09:13:31Z

Make sure these boxes are checked before submitting your pull request - thank you!

All coding styles are fulfilled. (How to check for cs issues?)
All tests are running locally. (How to run the test?)
Necessary update hooks are provided.
User roles have correct access for new introduced permission.
Every thunder module has a README.md in its root. Follow this guidelines, but we don't need every topic.
Code is covered with well-balanced amount of inline comments.

If you are really awesome, then your feature is covered by additional tests. Well done!

chrfritsch

"Liveblog paragraphs" it's not possible to enable that module in Thunder

chrfritsch · 2017-03-08T09:48:01Z

modules/thunder_liveblog/thunder_liveblog.install

+    'revert liveblog revisions',
+    'view liveblog revisions',
+  ];
+  $config = $config_factory->getEditable('user.role.editor');


Use a for-loop over the roles

chrfritsch · 2017-03-08T09:55:59Z

src/Plugin/Thunder/OptionalModule/Liveblog.php

+
+    $form['thunder_liveblog']['description'] = [
+      '#type' => 'item',
+      '#markup' => $this->t('Register a new account at <a href=":pusher_url" target="_blank">:pusher_url</a>, create a new app and note down your keys. You can provide theme right here or at a later stage on the liveblog settings form',


Missing dot at the end

Please mention that the pusher app must be registered in the US cluster. Also add that note to the liveblog settings page.

You don't have to register it in the US cluster, but I will add to the description, that you have to note down your cluster.

chrfritsch · 2017-03-08T10:02:04Z

src/Plugin/Thunder/OptionalModule/Liveblog.php

+      '#title' => t('Secret'),
+    ];
+
+    $form['thunder_liveblog']['pusher_cluster'] = [


Cluster field should be a select box, like it is on pusher app creation. Also add that to the liveblog settings page

Yeah, I also thought about that. Unfortunately, I can't get the available clusters from the Pusher API, so we would have to track changes there and modify it, each time.

I filed a ticket https://support.pusher.com/hc/en-us/requests/10412.
When someone opens up a new app, he also gets the cluster as a string, so I think it is ok for now:

chrfritsch · 2017-03-08T10:12:11Z

modules/thunder_liveblog/thunder_liveblog.info.yml

@@ -0,0 +1,17 @@
+name: 'Thunder Liveblog'


Add a "Liveblog" paragraph into thunder_liveblog to make it possible to integrate a liveblog into an article

hmm, this wasn't planned like that, since the liveblog lazyloads, so it must be always added to the end and it should be easily possible to set the "live" flag.

I would propose adding paragraphs to the Liveblog Content Type

Or I could add an entity reference to liveblog at the end of an article. The downside of this could be that you can't easily mark your liveblogs in a special way on the frontpage, or make a view with all articles which have a liveblog.

mtodor

In general this looks good. I have tested it drush + composer install, with initial install and with post install, with settings provided in install and after. Worked in all cases.

There are few things that I have found. I don't know what is part of liveblog module, what for integration, but here is list:

[LB] When I make big post (>10kb), limit for Pusher is 10kb, error occurs in log, that it can't be pushed.
But in live blog form I get success message: "Liveblog post was successfully created."
In log: "Failed to send a message to Pusher. See the request log..."
And also, since it's not pushed to pusher, user does not see post until page is reloaded.
-> maybe it should be considered to notify editor about error occurred. Best would be to not create post in Drupal, if it's not pushed to pusher. In general this should be atomic action, so post is successfully created only if it's created properly in both places -> Pusher + Drupal.
[LB] When "header" (liveblog content) is changed, it would be nice that it's also changed in live stream page.
[Integration] When I post some media paragraph, there is title above it: "Embed media". I'm not sure is that needed and can we hide label?
[Integration] I would be nice to add description for Liveblog content type.
[LB] Looks like that sometimes content is not loaded properly. Fe. If I create 2 posts really quickly (withing same minute) with titles 123 and 456 (in that order). Post will be correct in stream, when I reload page order is reverse and 123 is displayed before 456.

mtodor · 2017-03-10T10:50:21Z

src/Installer/Form/ModuleConfigureForm.php

-    foreach ($this->optionalModulesManager->getDefinitions() as $provider) {
+    $providers = $this->optionalModulesManager->getDefinitions();
+
+    uasort($providers, static::sortWeights());


This is a bit confusing -> when I see sortWeights(), I would expect that method will sort weights, but actually it returns function.

I would suggest if you want to have helper function, just pass array to it and do everything inside. Then you can say static::sortByWeights($providers); -> you have to accept array as reference (&$array), what is ugly, but that's case with all sort functions.

tjwelde · 2017-03-13T15:31:40Z

@mtodor Regarding 2) Which header do you mean? From an individual Liveblog Post?

mtodor · 2017-03-13T16:00:50Z

@goldquest Regarding 2) That's why I quoted it there ("header"), because I don't know how to describe it.

2nd try: What is displayed when you don't have any post -> that's liveblog content (static liveblog part). When you click on Edit on liveblog from content list page.

Now using it as a thunder integration module

Let the user configure pusher on the installation screen

Background: Thunder demo must be installed first, since it has its IDs hardcoded.

tjwelde · 2017-03-16T14:01:42Z

is adressed in seperate ticket
- fixed: post not saved and error shown
is adressed in seperate ticket
fixed
fixed
is adressed in seperate ticket

mtodor · 2017-03-20T13:16:22Z

From my point of view, this looks good.

chrfritsch suggested changes Mar 8, 2017

View reviewed changes

mtodor reviewed Mar 10, 2017

View reviewed changes

mtodor added needs work feature labels Mar 10, 2017

tjwelde mentioned this pull request Mar 13, 2017

Pusher: No error shown on size limit galaniprojects/liveblog#84

Open

tjwelde mentioned this pull request Mar 13, 2017

Inconsistent order, when posts are submitted simultaneously galaniprojects/liveblog#85

Open

tjwelde mentioned this pull request Mar 13, 2017

Push changes of liveblog article galaniprojects/liveblog#86

Open

tjwelde and others added 21 commits March 13, 2017 19:24

Include liveblog with thunder

e18900c

Liveblog: use newer version for make workflow

baf9593

moved liveblog_paragraphs into the distribution.

b38e5fa

Now using it as a thunder integration module

Liveblog: install Pusher by default

09ec0f5

Add Liveblog to the OptionalModules.

acb5359

Let the user configure pusher on the installation screen

sort optional modules by weight.

b3b8c5b

Background: Thunder demo must be installed first, since it has its IDs hardcoded.

added pusher library to make workflow

9e6f8ed

required core version was missing

0b48f40

liveblog has a dependeny to simple_gmap

f7440dd

only use pusher as notification channel, if necessary config is set

6800c45

fixed phpcs issues

d003c7a

fix phpcs issues with project prefixing

cdd73a8

added readme to thunder_liveblog

64e8bc0

add permissions for liveblog

9224066

use for-loop for adding permissions

d2e254a

added info about noting down cluster info

0554a29

update liveblog

daa9e63

Liveblog integration: replace body with paragraphs

ba19362

Added testing of configuration for Live Blog integraton

e5a8eca

Liveblog: resolve schema issues

c7ba264

removed thunder_liveblog from the schema check

fca6096

convert optional module sorting from callback to sorting function

050f61d

tjwelde force-pushed the feature/liveblog branch from ad4ab83 to 050f61d Compare March 13, 2017 18:24

tjwelde mentioned this pull request Mar 16, 2017

Liveblog content-type description galaniprojects/liveblog#89

Closed

tjwelde added 2 commits March 16, 2017 14:48

fix index region missing

72d29ad

hide labels for filed_paragraphs and field_embed_media

67c010b

update liveblog (node-description / show pusher errors)

f4bbde7

tjwelde added needs feedback and removed needs work labels Mar 16, 2017

Merge branch 'develop' into feature/liveblog

c24f385

chrfritsch approved these changes Mar 27, 2017

View reviewed changes

chrfritsch merged commit 5125cc3 into develop Mar 27, 2017

chrfritsch deleted the feature/liveblog branch March 27, 2017 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/liveblog #261

Feature/liveblog #261

tjwelde commented Mar 7, 2017 •

edited

Loading

chrfritsch left a comment

chrfritsch Mar 8, 2017

chrfritsch Mar 8, 2017

chrfritsch Mar 8, 2017

tjwelde Mar 8, 2017

chrfritsch Mar 8, 2017

tjwelde Mar 8, 2017 •

edited

Loading

tjwelde Mar 8, 2017

chrfritsch Mar 8, 2017

tjwelde Mar 8, 2017

tjwelde Mar 8, 2017

tjwelde Mar 8, 2017

mtodor left a comment •

edited

Loading

mtodor Mar 10, 2017

tjwelde commented Mar 13, 2017

mtodor commented Mar 13, 2017

tjwelde commented Mar 16, 2017 •

edited

Loading

mtodor commented Mar 20, 2017

Feature/liveblog #261

Feature/liveblog #261

Conversation

tjwelde commented Mar 7, 2017 • edited Loading

chrfritsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwelde Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtodor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwelde commented Mar 13, 2017

mtodor commented Mar 13, 2017

tjwelde commented Mar 16, 2017 • edited Loading

mtodor commented Mar 20, 2017

tjwelde commented Mar 7, 2017 •

edited

Loading

tjwelde Mar 8, 2017 •

edited

Loading

mtodor left a comment •

edited

Loading

tjwelde commented Mar 16, 2017 •

edited

Loading