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 jobs restapi to split up id and name functionality #355

Merged
merged 19 commits into from
Jan 23, 2024

Conversation

alexb1200
Copy link
Contributor

@alexb1200 alexb1200 commented Jan 5, 2024

Refactors the job endpoint to split up functionality of name and id requests. Includes minor changes to the unit tests to reflect those changes. Resolves #313

Changes

  • split by name functionality into a sperate service class
  • merge submit and create into one function called create
  • remove inline type annotations
  • add docstrings to service class
  • remove and unit tests for deprecated functions

@alexb1200 alexb1200 marked this pull request as ready for review January 5, 2024 19:40
@jkglasbrenner
Copy link
Collaborator

Going to begin my review, to start I'm going to rebase on dev and force-push to ensure all the tests are still passing.

@jkglasbrenner
Copy link
Collaborator

Pushing some updates to this branch. Most of them are minor stylistic things that weren't worth doing a code change request for (see end of comment for a list). The non-stylistic stuff I found in my review was the following, along with how I addressed them.

  • The Swagger Docs started having problems by including the PUT verb. The docs suggested the user could edit far more than the status field with a PUT, and for POST, it suggested users could set the initial value for status, neither of which we want. To fix this, I divided JobSchema into two classes, JobBaseSchema and JobMutableFieldsSchema, and then recombined them into JobSchema using multiple inheritance.
  • The @login_required decorator was missing from the new task engine job submission endpoint, so I added it.
  • I split the job submission logic for the new task engine into its own service class, JobNewTaskEngineService.

The minor stylistic stuff:

  • Ensuring that there is a consistent naming scheme across the Resource endpoint classes. The declarative task engine submission endpoint (and associated Schema class) was named in a different style, so I updated that.
  • There are some places where I combined the cast() and function return into a single line instead of splitting them across two lines. From what I can see, that's the general style we're following in the other refactored endpoints.
  • Some very minor whitespace stuff (removed some new lines here and there)
  • There were some redundancies with input checking and validation, for example the permitted status values are already validated by the Marshmallow Schema. There were also a couple of opportunities to switch to using an error_if_not_found=True argument. The code was updated in these circumstances.
  • Some of the docstrings documented raising errors that aren't found in the body of the method. In some cases, the error could be raised implicitly from another method call. That can be difficult to keep track of, so I think we can standardize on only documenting errors that are explicitly raised to keep it simpler.

If all the tests pass, then I think this will be good for a merge.

@jkglasbrenner jkglasbrenner merged commit fbdc3ba into dev Jan 23, 2024
23 checks passed
@jkglasbrenner jkglasbrenner deleted the abyrne-refactor-jobs branch January 23, 2024 21:57
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.

2 participants