Mapping: don't reset all targets [#853194]

Comment	File	Size	Author
#7	853194-7_reset_only_used_targets.patch	5.03 KB	alex_b

#1	853194-1_reset_only_used_targets.patch	1.96 KB	alex_b

Comment #1

alex_b CreditAttribution: alex_b commented 13 July 2010 at 19:58

Priority:	Normal	» Critical
Status:	Active	» Needs review

File	Size
853194-1_reset_only_used_targets.patch	1.96 KB

Patch adjusts reset so that only targets that are actually used in mappings are reset.

This patch also adds uid as target, which would break existing tests as map() resets all mapping targets an the test checks whether a change in author survives an update.

Log in or register to post comments

Comment #2

andrewlevine CreditAttribution: andrewlevine commented 13 July 2010 at 21:57

Taking a quick look this should be correct. I am going to test this patch on my application when I am done writing some unit tests for it.

Log in or register to post comments

Comment #3

andrewlevine CreditAttribution: andrewlevine commented 15 July 2010 at 15:39

Status:

Needs review

» Reviewed & tested by the community

Took a closer look and it looks good. Applied the patch and ran it through the custom unit tests for the site I'm working on (tests if some content using mappings was imported correctly) and that worked as well. RTBC

On a side note, if I updated Feeds to HEAD some 200+ unit tests fail...not sure why

Log in or register to post comments

Comment #4

Anonymous (not verified) CreditAttribution: Anonymous commented 15 July 2010 at 20:13

Thanks, I was worried about losing the "timestamp", this patch fixed it.

Log in or register to post comments

Comment #5

alex_b CreditAttribution: alex_b commented 16 July 2010 at 13:55

Thank you so much for the review. I will commit this on the weekend. FYI, I think we found another regression from #840626: #855222: Beta2 node processor overrides 'published' flag default

Log in or register to post comments

Comment #6

andrewlevine CreditAttribution: andrewlevine commented 16 July 2010 at 14:13

Alex, it is likely the #855222: Beta2 node processor overrides 'published' flag default regression will be solved by this patch, no? As long as the mapping is not set up in the feed.

Log in or register to post comments

Comment #7

alex_b CreditAttribution: alex_b commented 18 July 2010 at 18:10

File	Size
853194-7_reset_only_used_targets.patch	5.03 KB

- Adds a test case for the bug reported in #855222.
- Running tests now, if all are passing, I will commit.

On a side note, if I updated Feeds to HEAD some 200+ unit tests fail...not sure why

My bad, I committed a presumably minor fix to the settings UI (remove default setting 'page' for content type in basic settings) which broke a score of tests. This is fixed now.