fix: encode in abstractlazilyencodablesection not returning updated string #78
Conversation
modules/cmpapi/src/encoder/section/AbstractLazilyEncodableSection.ts
Outdated
Show resolved
Hide resolved
ankitonetrust
force-pushed
the
gpp-string-creation-fix-master
branch
from
91136b8 to
76324c1
Compare
ankitonetrust
force-pushed
the
gpp-string-creation-fix-master
branch
from
76324c1 to
04c7b30
Compare
| @@ -242,6 +242,7 @@ export class GppModel { | |||
| let sectionName = Sections.SECTION_ORDER[i]; | |||
| if (sections.has(sectionName)) { | |||
| let section = sections.get(sectionName); | |||
| section.setIsDirty(true); | |||
There was a problem hiding this comment.
@ankitonetrust - Please use the same method in all the places where isDirty is getting set directly. I suggest using a new method setIsDirty in GppModel, which will be responsible for setting the isDirty flag and syncing the same flag in AbstractLazilyEncodableSection as well.
There was a problem hiding this comment.
Hey @dev-spandey there is no dependency for isDirty in AbstractLazilyEncodableSection other than encode method and the same file takes care of setting isDirty false.
There was a problem hiding this comment.
@ankitonetrust I've been doing a little digging and I think to avoid the issue this change causes with the decoding is to move this line of code to line 114 of this same file. That way it only gets set for the specific task of setting field values, which was described in the original bug.
|
Is there any timeline for this fix to go live? This is a critical one. |
|
This fix does solve the original issue in my testing, but has potential implications for TC strings when TCF is included. I've submitted a separate bug for that issue that seems, at least partially, related to this issue. #79 |
| @@ -242,6 +242,7 @@ export class GppModel { | |||
| let sectionName = Sections.SECTION_ORDER[i]; | |||
| if (sections.has(sectionName)) { | |||
| let section = sections.get(sectionName); | |||
| section.setIsDirty(true); | |||
There was a problem hiding this comment.
@ankitonetrust I've been doing a little digging and I think to avoid the issue this change causes with the decoding is to move this line of code to line 114 of this same file. That way it only gets set for the specific task of setting field values, which was described in the original bug.
|
@HeinzBaumann this still needs my change request or it will continue to break the decode |
@gilluminate so basically your PR(#80) is going to fix the TCF related issue even without applying your change request of moving "section.setIsDirty(true)" to line 114. |
|
@ankitonetrust In my testing, with #80 applied and this PR applied as is, the TCF string being generated is an invalid string altogether (plugging it in to the decoder at https://iabgpp.com throws an error "DecodingError: Unable to decode TcfEuV2PublisherPurposesSegment 'YAAAAAAA'") I do see the point of not wanting to call it multiple times, but the fix in it's current state appears to still cause problems for the TCF string |
encode method in abstractlazilyencodablesection is not returning updated gpp string as dirty was set to false always