[FEATURE] [AUT-3574] Change User Perspective according to the login flow

[andreluizmachado]

What's changed

• Removed FF FEATURE_FLAG_TAO_AS_A_TOOL, the user experience should change dynamically now
• Implementing the interface AuthoringAsToolConfigProviderInterface. Tao Core uses this info to apply proper user experience

Please also check: oat-sa/tao-core#3983, https://github.com/oat-sa/tao-portal/pull/740

Refs: https://oat-sa.atlassian.net/browse/RFE-847
Refs: https://github.com/oat-sa/tao-portal/pull/720
Refs: https://github.com/oat-sa/tao-portal/pull/740

Demo

Feature

Screen.Recording.2024-03-19.at.09.25.21.mov

Fallback

Screen.Recording.2024-03-19.at.10.07.32.mov

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 58.33333% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 31.73%. Comparing base (97aeffc) to head (f7f8213).

Files	Patch %	Lines
...els/classes/ServiceProvider/LtiServiceProvider.php	0.00%	11 Missing ⚠️
models/classes/DynamicConfig/LtiConfigProvider.php	83.33%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #406      +/-   ##
=============================================
+ Coverage      31.32%   31.73%   +0.40%     
- Complexity       842      850       +8     
=============================================
  Files            106      107       +1     
  Lines           2953     2978      +25     
=============================================
+ Hits             925      945      +20     
- Misses          2028     2033       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bartlomiejmarszal]

I have similar comments to:
oat-sa/tao-core#3983 (review)

[Karol-Stelmaczonek]

• New code is covered by tests (if applicable)
• Tests are running successfully (old and new ones) on my local machine (if applicable)
• New code is respecting code style rules
• New code is respecting best practices
• New code is not subject to concurrency issues (if applicable)
• Feature is working correctly on my local machine (if applicable)
• Acceptance criteria are respected
• Pull request title and description are meaningful

[bartlomiejmarszal]

What you are trying to achieve is to provide different implementation of AuthoringAsToolConfigProviderInterface* that will contain reference to Lti Claim with a fallback to configuration values.

I don't agree with using previous service implementation from class properties as config fallback. Instead I would recommend to extend class, override methods or move common methods into abstract class.

• suggested name change

[bartlomiejmarszal]

Instead of decorating I would suggest to overload implementation of AuthoringAsToolConfigProviderInterface*

• recommended name change

[andreluizmachado]

Hello @bartlomiejmarszal

Embracing composition over method overloading aligns with the Liskov Substitution Principle, a fundamental tenet of object-oriented design. By favoring composition, we uphold the principle's mandate for substitutability, enabling interchangeable components that adhere to a consistent interface. This approach fosters greater code reliability, extensibility, and adaptability, as it encourages the creation of cohesive and loosely coupled modules that can be easily substituted or extended without compromising the integrity of the system.

[bartlomiejmarszal]

Suggested change

	public const LTI_REDIRECT_AFTER_LOGOUT_URL = 'authoringSettings.redirectAfterLogoutUrl';
	public const LTI_REDIRECT_AFTER_LOGOUT_URL = 'authoringSettings.redirectAfterLogoutUrl';

[bartlomiejmarszal]

Hello @bartlomiejmarszal

Embracing composition over method overloading aligns with the Liskov Substitution Principle, a fundamental tenet of object-oriented design. By favoring composition, we uphold the principle's mandate for substitutability, enabling interchangeable components that adhere to a consistent interface. This approach fosters greater code reliability, extensibility, and adaptability, as it encourages the creation of cohesive and loosely coupled modules that can be easily substituted or extended without compromising the integrity of the system.

IMO this implementation violate composition pattern as it using multiple implementation of itself by class property. I think for this purpose better fit would be using method overload

[andreluizmachado]

Hello @bartlomiejmarszal
Respectfully, I must disagree with your assertion. Composition and Aggregation aren't merely patterns but fundamental concepts in object-oriented programming. Thus, I don't perceive their usage as violating any principles; rather, it's a deliberate design choice.

Regarding your suggestion of method overloading, I'd like to delve deeper: which specific method are you proposing to overload? It's essential to consider the context and implications of such a decision within our design framework.

I think what you are proposing is to use inheritance instead of Aggregation, right???
In case I am right, I preferred to use the aggregation with the Decorator pattern because I could have different ways of getting the configurations (via env var, via auth2, via the session, via db...) and I could easily mount a list of priorities (which one to load first otherwise fallback to the next). With inheritance, we would lose this flexibility.

[github-actions]

Version

Target Version	15.19.0
Last version	15.18.0

There are 0 BREAKING CHANGE, 3 features, 10 fixes