Refactor AdminSetCreateService to work with both AdminSet and AdministrativeSet #5224
Conversation
elrayle
force-pushed
the
as_create_service
branch
from
51c2532
to
def1cf8
Compare
|
This PR is ready for review in spite of the CCI failures. The failures are the same as those seen on other PRs. Branch |
elrayle
force-pushed
the
as_create_service
branch
from
def1cf8
to
c1a5d59
Compare
This is in prep of the next step that will be able to create a `Hyrax::AdministrativeSet`.
Since this method doesn’t do any of the real work, only check that it passes through. The real work will be tested in the method that performs the work.
Since this method doesn’t do any of the real work, only check that it passes through. The real work will be tested in the method that performs the work.
update comments to reference `Hyrax::AdminstrativeSets` instead of `AdminSet`
elrayle
force-pushed
the
as_create_service
branch
from
c1a5d59
to
3a000f3
Compare
| @@ -1,38 +1,34 @@ | |||
| # frozen_string_literal: true | |||
| RSpec.describe Hyrax::AdminSetCreateService do | |||
There was a problem hiding this comment.
i think it would be nice to rebase these changes on #5213. some of the changes overlap, but i think that does a more complete job of fixing up the stubs, indirection, and multi-expectations in these specs.
i'd be willing to take on that rebase today if that PR could be reviewed.
There was a problem hiding this comment.
I am open to even more refactoring of the tests. It would be easier to review another round of refactoring if this PR were merged with the tests as is and follow that with an update to #5213 or a new PR that makes additional changes.
There was a problem hiding this comment.
i think reworking #5213 in light of these changes would amount to doing the work in it over again. on the other hand, the substantive changes in this PR are in another file, and it will likely continue to pass, or possibly require minor adjustments, if rebased on #5213.
i feel willing to do the work involved in latter, but not the former.
There was a problem hiding this comment.
My preference to overlay the #5213 changes is that the specs are rearranged in this PR to closer match that actual methods. The primary tests are only run for #create! since it is the only method that does any work beyond passing through data related to the new admin set. Other methods, including a few new methods, are only tested to ensure they pass through expected data and return expected values.
I am taking on the work to overlay #5213 over this PR in PR #5227.
`.call` and `#create` methods return Boolean and may hide runtime errors. Instead, use `.call!` and `#create!` respectively.
This is part of the work to address Issue #5130 (Val MVP: Create AdministrativeSet as a valkyrie resource through UI ).
Refactors:
#create_default_admin_setmethod to private#create_default_admin_set!methodcall!andcreate!returning aHyrax::AdministrativeSetinstead ofBooleanas returned by#calland#createHyrax::AdministrativeSetby default, but continue to support ActiveFedoraAdminSet@samvera/hyrax-code-reviewers