[FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled #392

andreluizmachado · 2023-10-16T16:06:07Z

We should redirect the user to the default login page in case:

The FF is not enabled
The LTI Platform config is missing

Please check the bug ticket for more details.

Demo

Current Behavior

current.mov

The fix

bugfix.mov

codecov-commenter · 2023-10-16T16:07:18Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (2e78ef5) 30.59% compared to head (af49cb5) 30.35%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #392      +/-   ##
=============================================
- Coverage      30.59%   30.35%   -0.25%     
- Complexity       817      822       +5     
=============================================
  Files            104      104              
  Lines           2853     2876      +23     
=============================================
  Hits             873      873              
- Misses          1980     2003      +23

Files	Coverage Δ
controller/AuthoringTool.php	`0.00% <0.00%> (ø)`

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

pribi

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
Pull request's target is not master

controller/AuthoringTool.php

bartlomiejmarszal

Review Checklist

controller/AuthoringTool.php

bartlomiejmarszal

My recommendation is to fallback into redirect after we catches exception from . getValidatedLtiMessagePayload method

controller/AuthoringTool.php

andreluizmachado · 2023-10-17T11:52:29Z

My recommendation is to fallback into redirect after we catches exception from . getValidatedLtiMessagePayload method

If I understood correctly this is what I am doing.

controller/AuthoringTool.php

bartlomiejmarszal · 2023-10-18T07:15:24Z

controller/AuthoringTool.php

+        try {
+            $message = $this->getValidatedLtiMessagePayload();
+        } catch (LtiException $exception) {
+            if ($exception->getMessage() !== self::LTI_NO_MATCHING_REGISTRATION_FOUND_MESSAGE) {


We should use an Exception code or Exception type to define this not a message string. This is a blocker for me.

Sometimes exceptions don't implement the code and its hard to add it, maybe that's the case. But of course better would be to use code of specific exception class to check.

Thanks @Karol-Stelmaczonek , yes, I do not have a code here. I would use it in case I have it, ofc

Can you make changes to the exception so it will be easy to recognise the type of exception?

I would avoid creating a new release on Lti1p3Core only for this.

bartlomiejmarszal

IMO this is the fallback mechanism and we could title this PR as add fallback when no registration available. This is an important change and we should be careful with our changes and we should describe them with clarity. Also, I would say it is a feature, not a bug.

controller/AuthoringTool.php

github-actions · 2023-10-18T09:28:14Z

Version

Target Version	15.14.1
Last version	15.14.0

There are 0 BREAKING CHANGE, 0 feature, 2 fixes

andreluizmachado · 2023-10-18T09:31:50Z

IMO this is the fallback mechanism and we could title this PR as add fallback when no registration available. This is an important change and we should be careful with our changes and we should describe them with clarity. Also, I would say it is a feature, not a bug.

It's not common to refer to a "fallback" as a "feature." Typically, a "fallback" is a contingency strategy or action triggered when something doesn't work as expected, or when a system cannot perform its primary task. For example, in programming, a "fallback" can be an alternative function that is executed if the main function fails. In security systems, a "fallback" can be a backup procedure in case the primary systems fail.

A "feature" is usually a planned and desired functionality of a system, product, or service that is designed to meet specific user needs. Features are intentional components of a system, while a "fallback" is a contingency response to unforeseen situations.

In summary, it's not appropriate to say that a "fallback" is a "feature" because they serve different roles in a system or product.

Refs: https://github.com//pull/394 Refs: oat-sa/extension-tao-lti#392 Rfes: https://oat-sa.atlassian.net/browse/SOLAR-387

fix: verify if feature is enabled and if platform config is available

44a8d2d

andreluizmachado requested review from pribi, bartlomiejmarszal and Karol-Stelmaczonek October 16, 2023 16:06

chore: fix CS

6fada3a

pribi approved these changes Oct 16, 2023

View reviewed changes

controller/AuthoringTool.php Outdated Show resolved Hide resolved

chore(fix): typo on phpdoc

c683b5a

bartlomiejmarszal requested changes Oct 17, 2023

View reviewed changes

controller/AuthoringTool.php Show resolved Hide resolved

controller/AuthoringTool.php Outdated Show resolved Hide resolved

bartlomiejmarszal requested changes Oct 17, 2023

View reviewed changes

controller/AuthoringTool.php Outdated Show resolved Hide resolved

controller/AuthoringTool.php Show resolved Hide resolved

bartlomiejmarszal reviewed Oct 17, 2023

View reviewed changes

controller/AuthoringTool.php Outdated Show resolved Hide resolved

fix: removed wrong inspection

b887031

refactor: variable name

af49cb5

bartlomiejmarszal reviewed Oct 17, 2023

View reviewed changes

controller/AuthoringTool.php Show resolved Hide resolved

andreluizmachado requested a review from bartlomiejmarszal October 17, 2023 15:16

bartlomiejmarszal reviewed Oct 18, 2023

View reviewed changes

bartlomiejmarszal requested changes Oct 18, 2023

View reviewed changes

bartlomiejmarszal reviewed Oct 18, 2023

View reviewed changes

controller/AuthoringTool.php Outdated Show resolved Hide resolved

bartlomiejmarszal reviewed Oct 18, 2023

View reviewed changes

controller/AuthoringTool.php Outdated Show resolved Hide resolved

andreluizmachado requested a review from bartlomiejmarszal October 18, 2023 09:03

andreluizmachado added 2 commits October 18, 2023 11:21

refactor: increased the log level to warning

3fad5ae

chore: removed phpdoc

5f5f497

andreluizmachado changed the title ~~[FIX][SOLAR-387] verify if feature is enabled and if platform config is available~~ [FIX][SOLAR-387] Add fallback when no registration available Oct 18, 2023

andreluizmachado changed the title ~~[FIX][SOLAR-387] Add fallback when no registration available~~ [FIX][SOLAR-387] Add fallback when no registration available on FF FEATURE_FLAG_TAO_AS_A_TOOL is disabled Oct 18, 2023

andreluizmachado changed the title ~~[FIX][SOLAR-387] Add fallback when no registration available on FF FEATURE_FLAG_TAO_AS_A_TOOL is disabled~~ [FIX][SOLAR-387] Add fallback when no registration available on FF TAO_AS_A_TOOL is disabled Oct 18, 2023

andreluizmachado changed the title ~~[FIX][SOLAR-387] Add fallback when no registration available on FF TAO_AS_A_TOOL is disabled~~ [FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled Oct 18, 2023

Karol-Stelmaczonek approved these changes Oct 18, 2023

View reviewed changes

andreluizmachado merged commit a9e1286 into develop Oct 20, 2023

andreluizmachado deleted the fix/SOLAR-387/error-if-authoring-simplification-is-config-missing branch October 20, 2023 14:30

andreluizmachado mentioned this pull request Nov 3, 2023

[BACKPORT] [SOLAR-387] Add fallback when no registration available #394

Merged

andreluizmachado added a commit to oat-sa/tao-community that referenced this pull request Nov 3, 2023

chore: updated tao lti extension to version 15.13.4.1

7df3bf8

Refs: https://github.com//pull/394 Refs: oat-sa/extension-tao-lti#392 Rfes: https://oat-sa.atlassian.net/browse/SOLAR-387

andreluizmachado mentioned this pull request Nov 3, 2023

Release 2023.11.1 oat-sa/tao-community#548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled #392

[FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled #392

andreluizmachado commented Oct 16, 2023

codecov-commenter commented Oct 16, 2023 •

edited

Loading

pribi left a comment •

edited

Loading

bartlomiejmarszal left a comment •

edited

Loading

bartlomiejmarszal left a comment

andreluizmachado commented Oct 17, 2023

bartlomiejmarszal Oct 18, 2023 •

edited

Loading

Karol-Stelmaczonek Oct 18, 2023

andreluizmachado Oct 18, 2023

bartlomiejmarszal Oct 18, 2023

andreluizmachado Oct 18, 2023

bartlomiejmarszal left a comment

github-actions bot commented Oct 18, 2023

andreluizmachado commented Oct 18, 2023

[FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled #392

[FIX][SOLAR-387] Add fallback when no registration available or FF TAO_AS_A_TOOL is disabled #392

Conversation

andreluizmachado commented Oct 16, 2023

Demo

Current Behavior

The fix

codecov-commenter commented Oct 16, 2023 • edited Loading

Codecov Report

pribi left a comment • edited Loading

Choose a reason for hiding this comment

bartlomiejmarszal left a comment • edited Loading

Choose a reason for hiding this comment

bartlomiejmarszal left a comment

Choose a reason for hiding this comment

andreluizmachado commented Oct 17, 2023

bartlomiejmarszal Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Karol-Stelmaczonek Oct 18, 2023

Choose a reason for hiding this comment

andreluizmachado Oct 18, 2023

Choose a reason for hiding this comment

bartlomiejmarszal Oct 18, 2023

Choose a reason for hiding this comment

andreluizmachado Oct 18, 2023

Choose a reason for hiding this comment

bartlomiejmarszal left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2023

Version

andreluizmachado commented Oct 18, 2023

codecov-commenter commented Oct 16, 2023 •

edited

Loading

pribi left a comment •

edited

Loading

bartlomiejmarszal left a comment •

edited

Loading

bartlomiejmarszal Oct 18, 2023 •

edited

Loading