TheProlog / prolog-use_cases

Use-case layer for Meldd/Prolog application.
0 stars 0 forks source link

Implement the "Respond to a Single Proposed Contribution" Use Case. #40

Closed jdickey closed 8 years ago

jdickey commented 8 years ago

As specified in the Wiki page, an Author may Accept or Reject a Proposed Contribution.

jdickey commented 8 years ago

The commit essay for Commit 3416571 should also have

jdickey commented 8 years ago

Hmm.

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).

That needs fixing, urgently.

jdickey commented 8 years ago

Well, we figured out the proposed-contribution entity bit, and then figured out how to replace form objects with collaborator and attribute value objects supporting a use case implementation that uses classes such as this one written using a functional command object idiom to get things done.

Many yaks were shaved as part of this epic side journey, and now we need to put the wool to good use as in, for example, Issue #44 — of which the restart of what was #42 should be the first such use case.

jdickey commented 8 years ago

The Effects of Success as listed in the Wiki specification are specified as side effects of the execu­tion, further implying that numerous collaborators (both the Contribution and Article Repositories and, in future, the mechanisms for updating ProRank, for sending "an email or other external notifi­cation", etc) must be specified to the use-case object. This is, at best, creaky, cumbersome design, and highlights the need for a notification collaborator more robust than the thin-wrapper-around-an-HTML-page "UI Gateway" that we have heretofore used.

Rather than sending notifications, the RespondToSingleProposal::Result class must, in future, in­clude a col­lection of the notifications that need to be sent; each of them having a sender, recipient and pay­load. When ProRank support is added, the Result must also include an indicator of which rule is to be used to adjust the Proposer's and Author's respective ProRank scores, such as :accepted, :marked_as_troll, etc.

For a successful result, the Result class must include the proposed-contribution entity passed in as a parameter, and the accept­ed- or rejected-contribution entity which is to be persisted to stor­age by the delivery mechanism. It must also include an Article entity in which the body content has been updated by a) removing the marker for the end of the Proposed Contribution and b) replacing the marker for the beginning of the Proposed Contribution with a marker for either an Accepted or Rejected Contribution as appropriate.

Why do this? This will sharply reduce, if not completely eliminate, possible failures of external col­labora­tors inducing a failure of the use-case instance which leaves the system in an inconsistent state. It elimi­nates dependence of the use case's successful completion on the reliable, correct function­ing of all col­laborators; should a failure occur, the use case can simply be restarted since it does not directly affect external state. Future use-implementations, as well as maintenance/rework of existing ones (pinging Issue #44) should also follow this pattern absent absolutely compelling arguments to the contrary. (Those "com­pelling arguments" should then induce opening of a "bug" issue, as either this pattern or that use case was not thought through and implemented as clearly as it ought to have been.)

Obviously, the Wiki spec for this use case is to be updated accordingly.

jdickey commented 8 years ago

Another bit to keep in mind with the (updated) spec pertains to the Result object returned from the #call method. To quote the spec:

The Result instance itself will be a simple value ob­ject, with no domain logic of its own.

The Result object returned from a call to #call may be used to verify that the use case is working correctly, but no tests are needed for the Result itself, as there is no logic to test.

jdickey commented 8 years ago

Yes, we're getting closer, but we don't yet have a coherent view of the implementation details relevant to the inputs and processing steps required for this use-case class. At least, not in a way that lets us do useful work within the use-case object itself.

Let's review. As collaborators, passed into the initialiser at present include

  1. article_repo is the Article Repository. As the use case was originally envisioned, it would have been used to query for the Article specified by the Contribution Proposal, and to persist that Article after its body content was updated with the resolved- (Accepted- or Rejected-) Contribution marker tag pair. If the persistence fails for some reason, then we risk introducing an inconsistent state into the delivery mechanism (and thus the app running within it);
  2. authoriser has two methods, #guest? and current_user. The use case should verify that the cur­rent user (the Member presently authenticated as being logged-in to the system) is the Author of the Article whose Contribution is being presented for resolution;
  3. contribution_repo is the Contribution Repository. As originally planned, the use case would persist the resolved Contribution. This introduces the same risks and complexity as the Article Repository dis­cussed above.

The initialiser is used to inject the collaborators into the use-case instance, introducing state but not in and of itself risking inconsistent system state; if the operating system-level process were to fall over be­fore entering #call, no harm would be done by restarting execution.

The #call method, as originally envisioned, does introduce such risk.

The parameters for #call are:

  1. The proposal entity (an Entities::Contribution::Proposed instance). This has attributes which con­vey all germane data regarding the proposal:
    1. the ID of the Article associated with the Proposal;
    2. the proposer of the Contribution;
    3. the endpoints and proposed_content for the Proposal;
    4. a unique identifier; and
    5. various metadata attributes.
  2. Some means of conveying the Author's decision to Accept or Reject the Proposed Contribution.

And that last is the crux of the problem. How do we get the decision from the Author? If the answer is "From the UI, before entering the use case", then that rather defeats the authentication and verification steps we've specified.

The alternative would be to pass in a functional object which itself interacts with the UI to determine the Author's response. Such an object, let's call it a Responder, would have a #call method which would take two parameters, both of which are themselves callable (in Ruby, procs or lambdas).

  1. The first, which must accept a single symbolic parameter, would be called if the Author successfully enters a response (e.g., :accept or :reject).
  2. The second would be called in the event of a failure, including if the Author cancels the interaction by whatever means are supported by the UI. Any parameters specified by the Responder's #call would be serialised as JSON and added to the use case Result's errors attribute.

What "risk" were we talking about before discussing the #call parameters? In earlier use cases, and in several examples of the idiom in other codebases and Gems (such as Mutations and, arguably to a lesser degree, Interactor), actions are assumed to be atomic but that isn't really enforced by the design; sure, if writing to the database fails, it can be rolled back (as shown explicitly in the Interactor demo), but what if the use case/interaction execution itself is interrupted? The "right answer" would seem to be "wrap the use case in a database transaction at the implementation-mechanism layer", but previous practice and a quick survey of GitHub will demonstrate that that's not always done. Should it be? Should that level of tran­isent mutability be tolerated and, if not, what is the feasible alternative? We don't yet have answers for that — which doesn't mean that we're happy with the status quo.

If anyone has any better ideas, now would be the time.

jdickey commented 8 years ago

Precondition 3 ("Author of an Article to which a Contribution has been Proposed must have Published one or more Articles" is covered by Precondition 1 (Article Author must be current user), isn't it? Striking.

Also, if this use case doesn't handle the condition where the Article Author is the Proposer of a Contribution, where is that handled? We seem to recall that the ProposeEditContribution use case does a rather half-hearted job of it...

jdickey commented 8 years ago

Not so fast.

The specified :article and :original_content attributes of the Result object don't yet exist. :weary:

jdickey commented 8 years ago

Item 6 on the Success list for Gem Release 1.0 presently reads

in its :article attribute, an Article entity whose body content contains a resolved-contribution marker tag pair at the beginning of the Contribution, and with the proposed-Contribution beginning and ending marker tag pairs removed;

A hurried read of that requirement might infer that we're persisting the updated Article to the Repository in the course of processing success; a more careful read would indicate that no such assumption is warranted; we're merely handing back the entity with body content as it would be persisted after success.

An even more careful read of the code for the Contribution::Proposed entity would reveal that we define a UUID as the identifier for a Proposal, and that this has been done since the first commit of the new, Dry::Types-based code. This clashes with. e.g., some[support code for Proposing an Edit Contribution, which simply uses a record index in the contribution repository as an index number.

The only reason we continue to get a green bar is that there are currently no integration tests that execute a sequence of use cases; the unit tests for each use case completely mock repositories.

This is, upon initial review, nontrivial; just the kind of basic error one would expect an overworked, exhausted solo development effort rushing at all possible speed (and then some) for an extended period of time (say, four and a half years,) to fall into. :mask:

jdickey commented 8 years ago

As of Commit e0bccee, UUIDs are used for Contribution IDs consistently. This obviates the need to tie a Contribution ID to a specific Article ID.

jdickey commented 8 years ago

In the spec, we define the :article attribute of the result entity as

an Article entity whose body content contains a resolved-contribution marker tag pair at the beginning of the Contribution, and with the proposed-Contribution beginning and ending marker tag pairs removed

This brings up the question of "what is a 'resolved-contribution marker tag pair' as opposed to 'beginning' and 'ending' MTPs?"

A begin-Proposed-Contribution marker tag pair looks like

<a id="contribution-(uuid)-begin">

and an end-Proposed-Contribution MTP looks like

<a id="contribution-(uuid)-end">

where "(uuid)" is replaced by a matching UUID, e.g., d1ca5230-03aa-0134-4506-3c0754526993.

So what should a resolved (accepted or rejected MTP look like? There won't be a separate "begin" and "end" tag pair for it; rather it would be

<a id="contribution-(uuid)-accepted">

or

<a id="contribution-(uuid)-rejected">

"Hang on," I can hear you thinking. "Why embed the resolution in the ID attribute?" To avoid a premature database lookup; we can render a marker of some type at the correct point which indicates that an accepted, rejected, or proposed contribution is at this point, and delay the details until actually needed if we so choose; we're not locked into an immediate database access each and every time we render that contribution marker.

Where to put the resolved-contribution marker? For expedience's sake, let's just arbitrarily say "replacing the begin-proposed-contribution marker" and remove the end-proposed contribution marker from the content of the entity's body. That would satisfy the requirement.

mitpaladin commented 8 years ago

yea, tag placement doesn't matter so long as its in the same paragraph (to support UI level view into past contributions by paragraph in the future)