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

Copy to draft feature #340

Merged
merged 3 commits into from Jul 2, 2018

Conversation

Projects
None yet
5 participants
@rolanyang
Contributor

rolanyang commented May 31, 2018

The request comes up every now and then for the ability to copy a previously sent message into draft.
This adds a button in the "Sent" campaign list to do so.

https://forums.phplist.com/viewtopic.php?p=39364

Updated the patch to work with the latest version.

Would be nice if this could be coded as a plugin, but there isn't a hook for it yet.

https://mantis.phplist.org/view.php?id=19263

@samtuke samtuke requested review from michield and xh3n1 May 31, 2018

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke May 31, 2018

Contributor

Brilliant - thanks for the PR 👍 It should be reviewed shortly.

Contributor

samtuke commented May 31, 2018

Brilliant - thanks for the PR 👍 It should be reviewed shortly.

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield May 31, 2018

Member

Yes, this has been requested many times. I have always been hesitant to add something like this, as I think it will make it easier to accidentally duplicate content to subscribers.

My preference would be to wrap this in a config option that can be enabled in the config file, but is not enabled by default. We can then warn about it in the comments on the config file.

Member

michield commented May 31, 2018

Yes, this has been requested many times. I have always been hesitant to add something like this, as I think it will make it easier to accidentally duplicate content to subscribers.

My preference would be to wrap this in a config option that can be enabled in the config file, but is not enabled by default. We can then warn about it in the comments on the config file.

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke May 31, 2018

Contributor

How about adding contextual help or a popup to the button to warn not to send duplicate content to subscribers?

Contributor

samtuke commented May 31, 2018

How about adding contextual help or a popup to the button to warn not to send duplicate content to subscribers?

@xh3n1

xh3n1 requested changes May 31, 2018 edited

Great PR 🚀
just one small fix: could you please use a copy icon instead of edit?
photo_2018-05-31_21-07-40

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield May 31, 2018

Member

@rolanyang before we can merge PRs from you, could you sign the CLA? Thanks

Member

michield commented May 31, 2018

@rolanyang before we can merge PRs from you, could you sign the CLA? Thanks

@rolanyang

This comment has been minimized.

Show comment
Hide comment
@rolanyang

rolanyang May 31, 2018

Contributor

CLA done.
I was looking for an icon closest to copy in ui/dressprow/images/16x16
It was either edit-1.png or library.png

One more thing to look over:

I don't know what the database fields repeatinterval, repeatuntil, requeueuntil, requeueinterval do. Perhaps it might be better to leave them out of the "insert" query. Also I'm noticing now that "processed" should probably be left out as well.

Contributor

rolanyang commented May 31, 2018

CLA done.
I was looking for an icon closest to copy in ui/dressprow/images/16x16
It was either edit-1.png or library.png

One more thing to look over:

I don't know what the database fields repeatinterval, repeatuntil, requeueuntil, requeueinterval do. Perhaps it might be better to leave them out of the "insert" query. Also I'm noticing now that "processed" should probably be left out as well.

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke May 31, 2018

Contributor

I think the fontawesome catalogue of icons should be at your disposal as well.

Contributor

samtuke commented May 31, 2018

I think the fontawesome catalogue of icons should be at your disposal as well.

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield May 31, 2018

Member

You may want to check out the repeatMessage function

https://github.com/phpList/phplist3/blob/master/public_html/lists/admin/connect.php#L1811

This is used when REPETITION is enabled and campaigns and automatically duplicated after they have been sent.

To make this feature work fully, you would need to duplicate attachments as well.

You won't be able to use that function, but you can verify the steps done there.

Member

michield commented May 31, 2018

You may want to check out the repeatMessage function

https://github.com/phpList/phplist3/blob/master/public_html/lists/admin/connect.php#L1811

This is used when REPETITION is enabled and campaigns and automatically duplicated after they have been sent.

To make this feature work fully, you would need to duplicate attachments as well.

You won't be able to use that function, but you can verify the steps done there.

@rolanyang

This comment has been minimized.

Show comment
Hide comment
@rolanyang

rolanyang Jun 1, 2018

Contributor

In regards to using the "copy" icon, implementing that would cascade changes down into the bootstrap.js, and a slew of css files one for each of the themes. The 16x16 image would have to be extracted out of the glyph's file too as Dressprow theme doesn't use woff2 fonts for the buttons. I'm not sure if you want to go down that route.

Contributor

rolanyang commented Jun 1, 2018

In regards to using the "copy" icon, implementing that would cascade changes down into the bootstrap.js, and a slew of css files one for each of the themes. The 16x16 image would have to be extracted out of the glyph's file too as Dressprow theme doesn't use woff2 fonts for the buttons. I'm not sure if you want to go down that route.

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke Jun 1, 2018

Contributor

I copied this branch and created a PR which replaces the edit icon with a copy one: #342

Contributor

samtuke commented Jun 1, 2018

I copied this branch and created a PR which replaces the edit icon with a copy one: #342

@rolanyang

This comment has been minimized.

Show comment
Hide comment
@rolanyang

rolanyang Jun 5, 2018

Contributor

Are there any remaining tasks that need to be done on my part?

Contributor

rolanyang commented Jun 5, 2018

Are there any remaining tasks that need to be done on my part?

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke Jun 5, 2018

Contributor

@rolanyang What about campaign attachments? Did you take a look at the logic for campaign repetition that michield mentioned?

Contributor

samtuke commented Jun 5, 2018

@rolanyang What about campaign attachments? Did you take a look at the logic for campaign repetition that michield mentioned?

@bramley

This comment has been minimized.

Show comment
Hide comment
@bramley

bramley Jun 6, 2018

Contributor

This is missing some fields on the message table - uuid, repeatinterval and requeueinterval.

Several fields are held on the messagedata table, not on the message table - finishsending, sendmethod, sendurl, campaigntitle and excludelist, so those should be copied too.

I think that people will expect the new draft message to be sent to the same lists as the original, so the rows on listmessage for the original should be also be copied.

See https://github.com/bramley/phplist-plugin-common/blob/master/plugins/Common/DAO/Message.php#L36 for how I implemented similar function for the Campaigns plugin.

Contributor

bramley commented Jun 6, 2018

This is missing some fields on the message table - uuid, repeatinterval and requeueinterval.

Several fields are held on the messagedata table, not on the message table - finishsending, sendmethod, sendurl, campaigntitle and excludelist, so those should be copied too.

I think that people will expect the new draft message to be sent to the same lists as the original, so the rows on listmessage for the original should be also be copied.

See https://github.com/bramley/phplist-plugin-common/blob/master/plugins/Common/DAO/Message.php#L36 for how I implemented similar function for the Campaigns plugin.

Modified and inserted additional insert queries per bramley's suggest…
…ions.

Now copies data from messagedata and listmessage tables.
@rolanyang

This comment has been minimized.

Show comment
Hide comment
@rolanyang

rolanyang Jun 8, 2018

Contributor

In testing, I noticed the Campaign Subject would be lost om the message copy and instead take on the Campaign Title unless the "subject" field was copied over in the phplist_messagedata table.

Contributor

rolanyang commented Jun 8, 2018

In testing, I noticed the Campaign Subject would be lost om the message copy and instead take on the Campaign Title unless the "subject" field was copied over in the phplist_messagedata table.

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield Jun 11, 2018

Member

how does this relate to #342 ?

Member

michield commented Jun 11, 2018

how does this relate to #342 ?

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke Jun 11, 2018

Contributor

#342 Makes an icon available for the "duplicate" action. Any button can use the button by using the corresponding css class.

Contributor

samtuke commented Jun 11, 2018

#342 Makes an icon available for the "duplicate" action. Any button can use the button by using the corresponding css class.

@samtuke

This comment has been minimized.

Show comment
Hide comment
@samtuke

samtuke Jun 11, 2018

Contributor

@rolanyang Are you able to fix that so the subject isn't lost?

Contributor

samtuke commented Jun 11, 2018

@rolanyang Are you able to fix that so the subject isn't lost?

@rolanyang

This comment has been minimized.

Show comment
Hide comment
@rolanyang

rolanyang Jun 11, 2018

Contributor
Contributor

rolanyang commented Jun 11, 2018

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield Jun 14, 2018

Member

@samtuke good to go for me, if we ensure some testing is done

Member

michield commented Jun 14, 2018

@samtuke good to go for me, if we ensure some testing is done

@xh3n1

xh3n1 approved these changes Jun 27, 2018

@michield

This comment has been minimized.

Show comment
Hide comment
@michield

michield Jun 30, 2018

Member

@samtuke do you want to remove the wip label? or is there still something outstanding. I actually think that not copying the attachment may be a good thing, otherwise you could end up accidentally sending the same attachments. In a way, that was the reason I was never in favour of a feature like this, but as a minimal implementation I guess it will improve UX a little.

Member

michield commented Jun 30, 2018

@samtuke do you want to remove the wip label? or is there still something outstanding. I actually think that not copying the attachment may be a good thing, otherwise you could end up accidentally sending the same attachments. In a way, that was the reason I was never in favour of a feature like this, but as a minimal implementation I guess it will improve UX a little.

@samtuke samtuke merged commit df4bd18 into phpList:master Jul 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment