✨ Implemented duplicate post functionality#16767
✨ Implemented duplicate post functionality#16767mike182uk merged 5 commits intoTryGhost:mainfrom mike182uk:duplicate-post
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #16767 +/- ##
===========================================
+ Coverage 56.17% 84.72% +28.54%
===========================================
Files 1695 692 -1003
Lines 105839 53588 -52251
Branches 12249 6864 -5385
===========================================
- Hits 59456 45402 -14054
+ Misses 45272 8115 -37157
+ Partials 1111 71 -1040
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I'm slightly cautious when using omit in code as it hides additional maintenance long-term. For example, when we add a new field to the posts table we'd have remember to exclude it here too if it's a field we don't want to copy over. On the other hand, we'd get a failure for the snapshot test and would need to update it, so there is some signal for us to check on this list in the future. What are your thoughts on this @mike182uk ?
There was a problem hiding this comment.
Yeh I had the same thought - Inversely you end up with the same problem when you pick the fields you want - If a new field is added that needs to be copied over, you would need to update the list (or manually set the value on the model etc.) - It felt more likely that there would be less fields that need excluding than need including (which would ease maintenance burden), but yeah either way the snapshot test should flag that there is data mismatch
ghost/api-framework/lib/headers.js
Outdated
There was a problem hiding this comment.
I think a cleaner way to pass this information would be allowing for a custom option in the endpoint definition itself. Checking for method names in the framework gives a code smell tbh. 🤔
We could introduce something like frame.location and determine the returned header base on the value there, this way we would not create additional coupling between framework and method names inside of endpoint definitions.
There was a problem hiding this comment.
Yeh i get what your saying with this - Looking at it, I think this might be better done as a refactor afterwards instead of part of this PR? If we remove the method check, we would need to go through all of the controllers and provide someway of letting this logic know it needed to generate the location header
There was a problem hiding this comment.
We don't have to go throught all controllers, just check if the new config value is present than use it and if not leave the previous logic as is.
naz
left a comment
naz
left a comment
There was a problem hiding this comment.
It's a good direction and I think we are 99% there just a few minor tweaks we'd need to do. Great job Mike 🚀
refs: TryGhost/Product#3139 TryGhost/Product#3140 - Added duplicate post functionality to post list context menu - Currently only a single post can be duplicated at a time - Currently only enabled via the `Making it rain` flag - Added admin API endpoint to copy a post - `POST ghost/api/admin/posts/<post_id>/copy/` - Added admin API endpoint to copy a page - `POST ghost/api/admin/pages/<page_id>/copy/`
refs TryGhost/Product#3179 - updated the post duplication icon - added a separator between the non-destructive and destructive actions - improved icon consistency
| const locationHeaderDisabled = apiConfigHeaders?.location === false; | ||
| const hasFrameData = ['add', 'copy'].includes(frame?.method) && result[frame.docName]?.[0]?.id; | ||
| const hasLocationResolver = apiConfigHeaders?.location?.resolve; | ||
| const hasFrameData = (frame?.method === 'add' || hasLocationResolver) && result[frame.docName]?.[0]?.id; |
There was a problem hiding this comment.
Yessss! No additional hardcoded method in the framework layer and gives us a clear path of migration to move away from hardcoded add when we find appetite for it 👍
naz
left a comment
naz
left a comment
There was a problem hiding this comment.
The copy post feature is good for prime time 
3 participants
refs: https://github.com/TryGhost/Team/issues/3139 https://github.com/TryGhost/Team/issues/3140
Making it rainflagPOST ghost/api/admin/posts/<post_id>/copy/POST ghost/api/admin/pages/<page_id>/copy/