Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweet Entity #3748

Merged
merged 26 commits into from Apr 19, 2017
Merged

Tweet Entity #3748

merged 26 commits into from Apr 19, 2017

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Mar 24, 2017

Q A
Bug fix? N
New feature? Y
Related user documentation PR URL mautic/documentation#167
Related developer documentation PR URL mautic/developer-documentation#54
Issues addressed (#s or URLs) /
BC breaks? N
Deprecations? N

Description:

Right now the Tweet Marketing Message (MM) Channel is stored as MM parameters, it's not a link to a independent entity like other channels are. This limits us on showing some stats for specific tweets, reusing tweets and so on. This PR creates a Tweet entity and the standard Mautic infrastructure around it like permissions who can create/edit/view (own) tweets, UI to view, create and edit tweets and so on.

Attached database migration will create the tweet table and migrates existing Tweets from MM and channel events to it and links those newly created tweets back to MM and channel events to where they belong.

Topic to discuss:

I'd like to start also a discussion about what fields the tweet entity should have. I added some fields like tweet_id, media_id, date_tweeted... which are not used now, but I think they will find its use in the future. This way we'll avoid another migrations. The inspiration about available tweet fields can be find in the twitter API docs: https://dev.twitter.com/overview/api/tweets
For example coordinates or place or in_reply_to_user_id. Should they be included?

Right now, after the tweet is sent, the tweet_id from Twitter and date_tweeted is being updated for the tweet entity. Although it will keep value only for the last sent tweet. The tweets serves now as a template where the same tweet can be sent to many contacts, so that's not probably the right place where to store those details.

Steps to test this PR:

  1. Make sure you have some MM channels with tweets, so you could ensure the migration works Duplicate the tweet text so you could see if the duplicates will get merged into one tweet entity.
  2. Make sure you have some Campaign Tweet actions, so you could ensure the migration works for them.
  3. Apply this PR.
  4. Clear the Mautic cache (php app/console cache:clear).
  5. Run the migrations (php app/console doctrine:migrations:migrate)
  6. Check the MM and Campaign actions where you used to have tweets, the tweet form should be replaced with tweet list select box and the right tweet should be preselected. You can also check the tweet table where the tweet entities are stored. In the description of each tweet you will have information about the source of the tweet. Something like Migrated from marketing message (1) channel (91). Also the fields like date_added, created_by and so on will be filled with the values from MM/campaign where the tweet was created.
  7. Try to create a new tweet from the select box. It should pre-select it in the tweet select box and if you save&close the MM, edit it again, it will persist there.
  8. Try to send a MM with a tweet with through a campaign.
  9. Try 6, 7 and 8 with the "Tweet contact" campaign action.
  10. Test the Tweets API endpoints by running the API library tests with this PR applied Tweet API endpoint added api-library#100
  11. Test the Tweet UI under Channels/Tweets. Try to create, edit, sort, paginate, search, delete, unpublish and whatever else the UI allows you to do.

@escopecz escopecz added the feature A new feature for inclusion in minor or major releases label Mar 24, 2017
@matishaladiwala matishaladiwala modified the milestone: 2.8.0 Mar 24, 2017
@escopecz escopecz added needs-documentation PR's that need documentation before they can be merged ready-to-test PR's that are ready to test labels Mar 28, 2017
@escopecz escopecz removed the needs-documentation PR's that need documentation before they can be merged label Mar 28, 2017
@dongilbert
Copy link
Member

I get the following error when running the migration:

 General error: 1364 Field 'favorite_count' doesn't have a default value

Should the fields favorite_count and retweet_count be defined as NOT NULL or DEFAULT NULL, or even DEFAULT 0?

@dongilbert dongilbert added the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 12, 2017
@dongilbert
Copy link
Member

I changed them to DEFAULT 0 and the migration ran successfully. Testing the rest of it now.

@escopecz
Copy link
Sponsor Member Author

Thanks for catching that, Don! I tested the migration several times. Weird it did not cause any trouble for me. I made the count columns nullable.

To retest, please delete the tweet_stats and tweets tables and remove the 20170323111702 row from the migrations table. Then you can run the migration again.

@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 12, 2017
@dongilbert
Copy link
Member

That worked @escopecz. Thanks!

@dongilbert
Copy link
Member

Unit tests pass and everything is looking good. 👍

@dongilbert dongilbert added the pending-test-confirmation PR's that require one test before they can be merged label Apr 12, 2017
@virlatinus
Copy link
Contributor

Tested this PR and it works great! Thanks John!

@virlatinus virlatinus merged commit 5694182 into mautic:staging Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature for inclusion in minor or major releases pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants