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

EZP-29586: Set ownership of copied Content Versions to the current User #2464

Merged
merged 2 commits into from
Dec 13, 2019

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Oct 10, 2018

Question Answer
JIRA issue EZP-29586
Bug/Improvement yes
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

This if followup with bugfix to this #2447
TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@andrerom
Copy link
Contributor

What do you mean by "Properly", what does this change do?

Personally I would expect copied content to retain version history incl who created each version, but if the code there is doing what I think it does it already resets created and modified date.

@ViniTou
Copy link
Contributor Author

ViniTou commented Oct 11, 2018

@andrerom
Yes, it overwrites version history as it expected to be and as is stated in comment to this jira issue.

@micszo
Copy link
Member

micszo commented Oct 12, 2018

What do you mean by "Properly", what does this change do?

@andrerom this comes from @SylvainGuittard's comment:

The account who copied the content item should be the owner of the content item and the contributor on all the versions.

here: https://jira.ez.no/browse/EZP-29586?focusedCommentId=231390&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-231390

While testing I observed that when admin was copying items created by test editor the contributor of the Published version was changed but it wasn't changed for Drafts or Archived versions. Examples below.

screen shot 2018-10-09 at 16 11 30

screen shot 2018-10-09 at 16 15 10

@gggeek
Copy link
Contributor

gggeek commented Oct 12, 2018

If I got it correctly, I would disagree with the requirement "The account who copied the content item should be the owner of the content item and the contributor on all the versions.". This means destroying real information and introducing bogus info - if Admin copies object X at version 5, I personally would not expect him to be shown as having created versions 1-4 (as in fact he never did create anything in the past). Either versions 1-4 should be dropped from the copy, and the copied object be created with a fresh creation timestamp at v1, or they should be retained with proper data.

Is this a case of altering the repository API to better suit the new GUI?

@SylvainGuittard
Copy link
Contributor

Hi @gggeek !
It is how eZ Publish works. When you copy a content item, you become the creator of all versions (archived and published).
I think customers are expecting consistency when they migrate from eZ Publish to eZ Platform.

@gggeek
Copy link
Contributor

gggeek commented Oct 12, 2018

Ah ok. BC before all, then. I can live with that.

Still, it would be good to have this behaviour not hardcoded in the repo, in case in the future someone wants to change/tweak it... (or at least make sure it is easy to introduce a 'shallow copy' operation that does not retain history)

@alongosz alongosz changed the title EZP-29586: Properly handle owner of old content versions when copying EZP-29586: Content copied by another user is still owned by original creator Oct 12, 2018
@alongosz alongosz changed the title EZP-29586: Content copied by another user is still owned by original creator EZP-29586: Set ownership of copied Content Versions to the current User Oct 12, 2018
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH this behavior does not make sense to me, but if this is what you want...

I took a liberty of changing PR's title that will go in as a commit message to have something more descriptive.

@emodric
Copy link
Contributor

emodric commented Oct 12, 2018

If this is indeed to preserve BC with legacy, I agree with @gggeek that this should be possible to enable/disable. I'd expect all the info preserved.

@andrerom
Copy link
Contributor

andrerom commented Oct 13, 2018

@gggeek and @emodric beat me to it, I also think we should consider letting API user decide, also since this breaks API behavior (todo: whatever the outcome of this is, lets document this on API php doc).

A. It could be an bool option to API, and we can discuss if it makes sense to break current API behavior and make it default enabled or not.

Coming from Git I don't expect versions to change, but if PM have insights into Editorial exceptions that says otherwise then I'm open to it.

B. From API side this would perhaps also be a good expectation:

  • copySubtree and copyContent should behave the same way where applicable
  • By default they only set owner id of the content (?? todo: check if copySubtree() does this)

As for being able to set creator/contributor of versions, current API is:

copyContent(ContentInfo $contentInfo, LocationCreateStruct $destinationLocationCreateStruct, VersionInfo $versionInfo = null)

When copying single content and specifying singular version to copy, we can make it possible to reset VersionInfo->creatorId (aka contributor in UI) so that the use case is possible. We can also make example on the API to showcase it.

As we could argue that in cases you want to mess with version info like who made it and so on, then you don't really care about keeping version history 🤷‍♂️

@crevillo
Copy link
Contributor

sorry to interrupt here,
regarding @SylvainGuittard's comment

It is how eZ Publish works. When you copy a content item, you become the creator of all versions (archived and published).

but do you mean you become the creator of the original content or the new created content after copy operation?

@alongosz
Copy link
Member

It is how eZ Publish works. When you copy a content item, you become the creator of all versions (archived and published).

but do you mean you become the creator of the original content or the new created content after copy operation?

@crevillo It's about ownership of new Content and all its Versions :)

@crevillo
Copy link
Contributor

@alongosz well, then i don't fully disagree with the current approach. i mean, when you copy a content, you are creating a new content too, aren't you?

At the end, once you have copied a content, how do you know in the future that the content B is a copy of content A? can you know it?

if you don't, creator of the original content may say "hey, i only created this but i don't have any clue who did this other one. It wasn't me..."

@alongosz
Copy link
Member

well, then i don't fully disagree with the current approach. i mean, when you copy a content, you are creating a new content too, aren't you?

Yes and I think this was justification for Legacy's behavior.

At the end, once you have copied a content, how do you know in the future that the content B is a copy of content A? can you know it?

No, this information is not stored anywhere.

if you don't, creator of the original content may say "hey, i only created this but i don't have any clue who did this other one. It wasn't me..."

So you're saying for you this PR's change is actually better? :)

My doubts are in the area of Versions, which should be a sort of historical record, not touched.

However @SylvainGuittard made a good point to me recently about owner/self Limitation which is about disallowing editing not own content. In this context the change from this PR might be needed.

Anyway, I think we're gonna have an internal meeting on this, so we'll keep you posted.

@crevillo
Copy link
Contributor

crevillo commented Oct 15, 2018

So you're saying for you this PR's change is actually better? :)
yes. imho, if you copy the content and because of that you're copying all versions, you should be the owner of all these versions too...

Anyway, i admit this might be argueable. other option could be keep the reference for the original creator and store somewhere who copied it. but that will open another bunch of possibilities too... :)

@gggeek
Copy link
Contributor

gggeek commented Oct 15, 2018

@crevillo the info about who is doing the copy gets stored in the fact that she becomes the new 'creator' of the content. What we are talking about here is only reassigning the creator of the old versions.

Other ways of doing that could arguably be possible, f.e.

  • add a new content version which corresponds to the copy (but then again "who has created this new version if the copy has not changed" ?)
  • remove all old versions and keep only the current (in fact in the ez4 packaging system you always get asked if you want to keep all content versions or only the current one. Generally annoying question, but ueful)

I think that there is no 'perfect' solution here, as different users will have different needs

@crevillo
Copy link
Contributor

@gggeek

the info about who is doing the copy gets stored in the fact that she becomes the new 'creator' of the content

yes, but two months later how do you know content B was created as a copy of A?

@andrerom
Copy link
Contributor

andrerom commented Oct 15, 2018

@crevillo So we already as @gggeek says re-assign owner of the content to the one copying.

What this changes is for creator of versions to behave like legacy, breaking (/cleaning up depending on perspective) how API has behaved, up until now to re-assign creator on those as well.

One of the arguments for not changing creator (on versions) is that you haven't really changed the content yet. If you do by means of editing it, the new version you create will be with you as creator, while archived versions will clearly show who originally created the older versions.

For me that is cleanest behavior for API by default, and we can accomplish the other use case by means of allowing passed VersionInfo to be set with different creator and document that.

@ViniTou
Copy link
Contributor Author

ViniTou commented Dec 5, 2019

@andrerom
@SylvainGuittard
@konradoboza

Can we move one way or another with it? ;)

@SylvainGuittard
Copy link
Contributor

@ViniTou

One of the arguments for not changing creator (on versions) is that you haven't really changed the content yet. If you do by means of editing it, the new version you create will be with you as creator, while archived versions will clearly show who originally created the older versions.

I agree with @andrerom on this but I still think the best option is to change the owner of all versions: BC first. :)

Regarding @crevillo request about "linking" the copied content to the original content is a good idea, but it's not the purpose of this PR. I will add this feature request to productboard.

@ViniTou ViniTou force-pushed the EZP-29586-copied-content-owner branch from 285dc2f to 0b6500f Compare December 9, 2019 10:08
@ViniTou ViniTou changed the base branch from 6.7 to 7.5 December 9, 2019 10:08
@ViniTou
Copy link
Contributor Author

ViniTou commented Dec 9, 2019

@alongosz
@SylvainGuittard
@ezsystems/qa-team

I have rebased this against 7.5, if we could do a second round of QA on this it would be great :)

@micszo micszo self-assigned this Dec 12, 2019
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Approved on eZ Platform v2.5.7.

@micszo micszo removed their assignment Dec 13, 2019
@lserwatka lserwatka merged commit 33e7998 into ezsystems:7.5 Dec 13, 2019
@lserwatka
Copy link
Member

@ViniTou could you merge it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants