openedx / xblock-lti-consumer

GNU Affero General Public License v3.0
28 stars 84 forks source link

fix: remove lms specific waffle check #312

Closed zacharis278 closed 1 year ago

zacharis278 commented 1 year ago

I'm running into two problems with this check when this code is run in edx-exams. Location isn't required on this model so location.course_key may fail but more importantly database_config_enabled can only run in the LMS. Checking if location exists first would technically fix this since a location implies this is running in the LMS (for now) but I'm coming to the conclusion that this check should probably not exist here.

All other instances of database_config_enabled occur within the lti_xblock.py which is totally fine to call into the LMS compatibility layer but baking this function into the data model feels like something that will catch us again later. The waffle flag already gates the UI behavior, do we really need this check on the model also?

Let know if I'm missing some good arguments for keeping this in, otherwise I propose we remove it.

codecov[bot] commented 1 year ago

Codecov Report

Base: 97.80% // Head: 97.80% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (492d978) compared to base (78cabcf). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #312 +/- ## ========================================== - Coverage 97.80% 97.80% -0.01% ========================================== Files 72 72 Lines 5917 5913 -4 ========================================== - Hits 5787 5783 -4 Misses 130 130 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `97.80% <100.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/openedx/xblock-lti-consumer/pull/312?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx) | Coverage Δ | | |---|---|---| | [lti\_consumer/models.py](https://codecov.io/gh/openedx/xblock-lti-consumer/pull/312/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-bHRpX2NvbnN1bWVyL21vZGVscy5weQ==) | `91.42% <ø> (-0.07%)` | :arrow_down: | | [lti\_consumer/tests/unit/test\_models.py](https://codecov.io/gh/openedx/xblock-lti-consumer/pull/312/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx#diff-bHRpX2NvbnN1bWVyL3Rlc3RzL3VuaXQvdGVzdF9tb2RlbHMucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openedx)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.