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

Deprecate startEngagementWithConfig method #835

Merged

Conversation

rasmustautsglia
Copy link
Contributor

@rasmustautsglia rasmustautsglia commented Nov 7, 2023

Jira issue:
https://glia.atlassian.net/browse/MOB-2817

What was solved?
This PR deprecates startEngagementWithConfig method and moves it to the deprecated methods file. This PR also replaces configure method with new one that takes Theme as a parameter. In addition, startEngagement method is deprecated and replaced with a new one that doesn't have Theme as parameter.

Release notes:

  • Feature
  • Ignore
  • Release notes (Is it clear from the description here?)
  • Migration guide (If changes are needed for integrator already using the SDK - what needs to be communicated? Add underneath please)

Additional info:

  • Tests fixed, added? Unit, acceptance, snapshots?
  • Logging necessary for future troubleshooting of customer issues added?

Screenshots:

Copy link
Contributor

@EgorovEI EgorovEI left a comment

Choose a reason for hiding this comment

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

I would also update Testing App to pass RemoteConfiguration using configure

GliaWidgets/Public/Glia/Glia.Deprecated.swift Outdated Show resolved Hide resolved
GliaWidgets/Public/Glia/Glia.Deprecated.swift Outdated Show resolved Hide resolved
@rasmustautsglia rasmustautsglia force-pushed the Deprecate-startEngagementWithConfig-method branch from 31f1975 to 6635dff Compare November 8, 2023 11:52
GliaWidgets/Public/Glia/Glia.Deprecated.swift Outdated Show resolved Hide resolved
GliaWidgets/Public/Glia/Glia.Deprecated.swift Outdated Show resolved Hide resolved
GliaWidgets/Public/Glia/Glia.swift Outdated Show resolved Hide resolved
GliaWidgets/Public/Glia/Glia.Deprecated.swift Outdated Show resolved Hide resolved
features: Features = .all,
sceneProvider: SceneProvider? = nil
) throws {
try startEngagement(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to store theme there?
self.theme = theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because if this is removed, the warning comes on and says this method is infinite recursion

@EgorovEI
Copy link
Contributor

I would also ask you to add unit test for applying RemoteConfiguration over Theme (few values would be enough). Just to ensure it's applied.

/// Deprectated, use ``Glia.startEngagement(engagementKind:in queueIds:features:sceneProvider:)`` instead.
/// Use ``configure(with configuration:uiConfig:assetsBuilder:completion:)`` to pass in ``RemoteConfiguration``.
@available(*, deprecated, message: """
Deprectated, use ``Glia.startEngagement(engagementKind:in queueIds:features:sceneProvider:)`` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 768c529

)
}

/// Deprectated, use ``Glia.startEngagement(engagementKind:in queueIds:features:sceneProvider:)`` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, Deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 768c529

/// Deprectated, use ``Glia.startEngagement(engagementKind:in queueIds:features:sceneProvider:)`` instead.
/// Use ``configure(with configuration:uiConfig:theme:assetsBuilder:completion:)`` to pass in ``RemoteConfiguration``.
@available(*, deprecated, message: """
Deprectated, use ``Glia.startEngagement(engagementKind:in queueIds:features:sceneProvider:)`` instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 768c529

Copy link
Contributor

@igorkravchenko igorkravchenko left a comment

Choose a reason for hiding this comment

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

Left comment about Theme recreation.

try Glia.sharedInstance.configure(with: configuration) { [weak self] result in
try Glia.sharedInstance.configure(
with: configuration,
theme: Theme()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does theme have to reconstructed here every time 'authenticate' is tapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because if I remove this parameter, Xcode picks the old configure method instead and deprecation warning appears.

@rasmustautsglia
Copy link
Contributor Author

!squash

@sm-deployer sm-deployer force-pushed the Deprecate-startEngagementWithConfig-method branch from e424849 to c7c4b99 Compare November 13, 2023 13:16
This PR deprecates startEngagementWithConfig method and moves it to
deprecated methods file

MOB-2817
@rasmustautsglia rasmustautsglia force-pushed the Deprecate-startEngagementWithConfig-method branch from c7c4b99 to c4f297e Compare November 13, 2023 13:19
@rasmustautsglia rasmustautsglia merged commit 4a9e607 into development Nov 13, 2023
8 checks passed
@rasmustautsglia rasmustautsglia deleted the Deprecate-startEngagementWithConfig-method branch November 13, 2023 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants