De-duplicate and fix proposed-edit-contribution entity.

[jdickey]

We have two proposed-contribution entities, Prolog::Entities::EditContribution::Proposed and Prolog::Entities::Proposal, and neither has a unique identifier for the contribution itself (which we've specified elsewhere as an ordinal number).

This was discovered as we were addressing Issue #40 with PR #41, causing us to stop work on #41 and fix this prior to continuing (actually, to closing that PR and opening a new one).

😩

[jdickey]

The two existing classes are _near-_duplicates of each other. Prolog::Entities::Proposal has a proposer attribute that EditContribution::Proposed calls user_name and, again, neither has any concept of sequence; they're each fragmentary value objects. We need a better value object, with the following attributes:

• article_id as an ArticleIdent entity;
• proposer, the Member Name of the submitter of the proposal;
• endpoints, as an integer range, obviously;
• proposed_content;
• justification, an optional string, defaulting to the empty string;
• proposed_at, an optional DateTime instance that defaults to DateTime.now; and, last but not least,
• sequence_number, a unique-within-the-article-history sequence number.

That sequence number must be supplied when instantiating this entity; since it's a value object, it "doesn't know how" to hit the database and generate the appropriate value automatically. Consequently, care must be taken by the instantiating code to use the correct value, as it's likely to be trusted by any code that accesses it after instantiation.

[jdickey]

Taking the previous comment into account, as well as this comment on Issue #28, we're going to create a new, "unifying", proposed-edit-contribution entity class. It will:

1. have the attributes identified immediately above;
2. not be Virtus-based;
3. have a matching Dry::Validation::Schema;
4. use a "parallel", non-Virtus-based article-id value object.

Hopefully, this is sufficiently clear and correct to begin poking at tomorrow.

[jdickey]

Well, we've _re-_duplicated edit-contribution (and article-id) entities as of Commit 524b93a.

Besides the Prolog::Entities::EditContribution::Proposed entity, the Prolog::UseCases::ProposeEditContribution::FormObject class directly uses the Virtus-based ArticleIdent. Therefore, the ProposeEditContribution use-case class is indirectly dependent on it, and its tests are directly dependent on ArticleIdent.

The QueryArticleProposedContributions use case is the only class that presently makes explicit use of the Virtus-based Prolog::Entities::EditContribution::Proposed entity; the remaining code that actually touches instances of the entity all use duck typing (for code) or mocks (for tests). This seems like it should be an easy conversion, leaving only the ArticleIdent class as a Virtus-based entity.

[jdickey]

And no, it isn't as bad as all that.

The only thing remaining in order to kill the original ArticleIdent, as of Commit b651265, is to replace the UseCases::ProposeEditContribution::FormObject.

"With what,", you might well splutter. "That's how that use case collects and validates its initialisation and #call parameters." Absolutely correct. (See the SRP violation there?)

ProposeEditContribution should instead use a Dry::Types-based attribute collection, with a Dry::Validation schema for validation instead. That's entirely practical, since the current use-case class uses the form object in four very separate, well-delimited modes:

• attribute assignment; partially in #initialize and completing the as-yet-unset attributes in #build_form (called by #call);
• validation proper; in #run_steps (again, called by #call);
• transferring error messages to the UI gateway in #transfer_errors (again, called from #run_steps which is called by #call); and finally
• converting the form object to a Hash as the parameter to contribution_repo.create.

Validation in the current object doesn't happen (and isn't meaningful) until we've entered #call and (in the current code) updated the form object. What we have now accretes two sets of attributes; #initialize saves off the contribution and article repositories, the UI gateway, an article, and an authoriser (the last two being saved in a not-yet-valid form object). (Immediately) later, #call updates the form object with endpoints, proposed content, and a justification string.

No wonder Reek bitches and moans about "too many instance variables!"

We have two "sets" of attributes that can largely be separated out:

• Collaborators include the article and contribution repositories, the authoriser, and the UI gateway; and
• Value attributes include the article, endpoints, justification, and proposed content.

The collaborators must meet certain expectations (as specified in the relevant APIs); validation of the value attributes is similarly straightforward.

This shouldn't be that hard to untangle...

[mitpaladin]

:) everything makes more sense after sleep

so of the 4 actions taken by the use case, ProposeEditContribution will then use attribute collection for attribute assignment, schema will be used for validation proper, then error messages and Hash goes inside the use case rather than the FormObject?

[jdickey]

If by "actions" you mean how the use-case class uses its form object?

There's going to be one value object containing the collaborator instances, and the use case will delegate to each of those collaborators via that value-object instance. There's going to be a second value object containing the value attributes germane to the use case (the second bullet in the last list above), also delegated to. We could do a validation schema to validate the collaborators; we will have one to validate the value attributes (passed in to #call). The value attributes are presumed to have originated in a form somewhere, and Dry::Types takes that into consideration.

So, to replace the form object, we'll have

• One value object for the collaborators;
• One value object for the value attributes; and
• A validation schema for the value attributes.

The existing form object is going away. Form objects are a great idea when you have clumps of attributes that need to be validated, then updated to redress errors, then re-validated; e.g., a typical Rails form interaction sequence. They're less helpful in a world of immutable data and functional command objects, which solve some other problems we've been butting up against regularly.

[jdickey]

IMPORTANT NOTE on Ruby and Rake

Beginning with Rake 11.1, the MiniTest Rake task is run with Ruby at its maximum warning level (equivalent to the CLI option -W2) which is extremely detailed and noisy, seemingly especially with third-party Gems. We've filtered and/or ignored the noise up to now, but -W2 causes an expectation

      expect { obj.call call_params }.must_be_silent

to fail:

Failure:
Prolog::UseCases::ProposeEditContribution::has a #call method that#test_0001_accepts a :justification parameter string [/Users/jeffdickey/src/rails/meldd/prolog-use_cases/test/prolog/use_cases/propose_edit_contribution_test.rb:213]
Minitest::Assertion: In stderr.
--- expected
+++ actual
@@ -1 +1,3 @@
-""
+"/usr/local/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/dry-types-0.7.1/lib/dry/types/struct.rb:62: warning: instance variable @schema not initialized
+/usr/local/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/dry-types-0.7.1/lib/dry/types/struct.rb:62: warning: instance variable @schema not initialized
+"

Yes, we could (and likely should) rewrite that expectation, since MiniTest, oddly, counts #must_be_silent as two assertions. But, in the meantime, you can revert to pre-Rake-11.1 behaviour by setting the environment variable

$ export RUBYOPT='-W1 -Itest'

😩

[jdickey]

As of Commit e62d0c5, we've whittled ProposeEditContribution::FormObject down to a shadow of its former self, its only remaining public non-initialiser method is #wrap_contribution_with, which still depends on the :article, :endpoints, and :proposed_content attributes.

But it's a decorator; it's not a form object. Let's implement it as one, with our newest service-class structure taking the three parameters and be done with it. Then, finally, we can take the sorry remains of ProposeEditContribution::FormObject and give it the final shredding and disposal it has so richly earned.