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

refactor: ORA data / UI layers #2044

Closed
wants to merge 105 commits into from
Closed

refactor: ORA data / UI layers #2044

wants to merge 105 commits into from

Conversation

nsprenkle
Copy link
Contributor

@nsprenkle nsprenkle commented Sep 5, 2023

TL;DR - Split of functionality in mixins into data / UI components.

JIRA:

What changed?

  • Moved data gathering pieces of mixins into xblock/apis
  • Created xblock/ui_mixins/legacy to replace existing view/handler behaviors from mixins, leveraging our new APIs.
  • Moved remaining non-core-xblock functionality from xblock to xblock/utils.
  • Updated references to changed file locations.

Developer Checklist

Testing Instructions

  • Unit tests passing
  • Manual tests

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

  • I've done a visual code review
  • I've tested the new functionality

FYI: @openedx/content-aurora

@nsprenkle nsprenkle force-pushed the aurora/bff-datalayer branch from ca0a68f to 7454568 Compare September 5, 2023 20:19
Using black --line-length=120
This allows more immediate access to static resources and avoids
bad pathing lookup
@codecov

This comment was marked as off-topic.

Included renaming submission path/context to get submission path/context
@nsprenkle nsprenkle marked this pull request as ready for review September 7, 2023 14:07
@nsprenkle nsprenkle requested a review from a team as a code owner September 7, 2023 14:07
nsprenkle and others added 3 commits September 8, 2023 11:36
Fixes an issue with pathing of resource_string
Refactor of ORA to data APIs and split UI layers
from openassessment.xblock.apis.step_data_api import StepDataAPI


class SelfAssessmentAPI(StepDataAPI):
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you not getting lint errors that these are missing docstrings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jansenk , those are disabled by our pylint config... though we should probably have them, right @muselesscreator?

Copy link
Contributor

@jansenk jansenk left a comment

Choose a reason for hiding this comment

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

This is such a massive PR, i really wish we could break it up somehow. That said, i can't find anyhting particularly objectionable herre

@jansenk
Copy link
Contributor

jansenk commented Sep 12, 2023

You've also got some lint failures

@nsprenkle
Copy link
Contributor Author

This is such a massive PR, i really wish we could break it up somehow. That said, i can't find anyhting particularly objectionable herre

I agree. I also got overruled 🙃

@nedbat
Copy link
Contributor

nedbat commented Sep 12, 2023

CLA check, please.

@nedbat
Copy link
Contributor

nedbat commented Sep 12, 2023

CLA check.

@nedbat nedbat changed the title refactor: ORA data / UI layers refactor: ORA data / UI layers. Sep 12, 2023
@nsprenkle nsprenkle changed the title refactor: ORA data / UI layers. refactor: ORA data / UI layers Sep 12, 2023
@nsprenkle
Copy link
Contributor Author

Closing and reopening to re-run jobs

@nsprenkle nsprenkle closed this Sep 12, 2023
@nsprenkle nsprenkle reopened this Sep 12, 2023
@nsprenkle
Copy link
Contributor Author

Replacing with new PR to fix stuck job

@nsprenkle nsprenkle closed this Sep 12, 2023
@nsprenkle nsprenkle mentioned this pull request Sep 12, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants