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.

Sign up for GitHub

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

Jump to bottom

[SP-8236] "https://" is not included in SPPropertyName if it is already there #357

Merged
merged 4 commits into from
Jan 26, 2022
Merged

[SP-8236] "https://" is not included in SPPropertyName if it is already there #357

merged 4 commits into from
Jan 26, 2022

Conversation

Nevazhnovu
Copy link
Collaborator

No description provided.

andresilveirah
andresilveirah reviewed Jan 25, 2022
View reviewed changes
Copy link
Member

@andresilveirah andresilveirah left a comment

Choose a reason for hiding this comment

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

@Nevazhnovu I believe this should not be added to the SDK. Property names should not include https:// . We only prefix it because of legacy reasons.

@Nevazhnovu
Copy link
Collaborator Author

@Nevazhnovu I believe this should not be added to the SDK. Property names should not include https:// . We only prefix it because of legacy reasons.

Nowadays we add https:// even if property name already include it. Such behaviour leads to "Unexpected /message response" errors. You sure?

@andresilveirah
Copy link
Member

Let me double check with some folks. I thought property names should not include https:// but it doesn't hurt checking.

@andresilveirah
Copy link
Member

@Nevazhnovu alright, I checked and we do have some properties with http://|https:// in the name. I think we should extend your fix to check for http as well. If it has it, we should prefix the prop name with https://.

@Nevazhnovu
Copy link
Collaborator Author

we should prefix the prop name with https://.
Just to be sure I understood right: "http://prop" -> "https://prop", right ?

@Nevazhnovu
Copy link
Collaborator Author

@andresilveirah replacing is done, pls validate

@andresilveirah
Copy link
Member

@Nevazhnovu I believe I wasn't clear. The logic should be:

  1. if the prop name starts with https:// or http://, it should remain as is
  2. else, we prefix it with https://

@Nevazhnovu
Copy link
Collaborator Author

@andresilveirah I got you. Commit is on the way, stay tuned

@Nevazhnovu
Copy link
Collaborator Author

@andresilveirah check it out

@andresilveirah
Copy link
Member

@Nevazhnovu wonderful. We just need 2 unit tests covering the changes ❤️

@Nevazhnovu
Copy link
Collaborator Author

@andresilveirah check it out

@andresilveirah
Copy link
Member

💯

@Nevazhnovu Nevazhnovu merged commit e59e89f into develop Jan 26, 2022
Nevazhnovu added a commit that referenced this pull request Jan 27, 2022
@Nevazhnovu
Merge branch 'develop' into SP-8555_tvOS_native_message_logo
7f44ed4 
* develop:
  [SP-8236] "https://" is not included in SPPropertyName if it is already there (#357)
  SP-8288 ott unexpected response message endpoint (#353)
  SP-8346 fix old unit tests (#356)
  SP-8330 appletv html crash (#352)

# Conflicts:
#	ConsentViewController/Classes/SourcePointClient/GDPRPrivacyManagerViewResponse.swift
#	ConsentViewController/Classes/SourcePointClient/SPPrivacyManagerRequestResponse.swift
@andresilveirah andresilveirah deleted the SP-8236_ott_unexpected_response_from_message_endpoint branch February 1, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresilveirah andresilveirah andresilveirah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Nevazhnovu @andresilveirah