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

Improve organisation and naming of controllers and methods #5225

Open
theseanything opened this issue Mar 5, 2021 · 2 comments
Open

Improve organisation and naming of controllers and methods #5225

theseanything opened this issue Mar 5, 2021 · 2 comments

Comments

@theseanything
Copy link
Contributor

As discussed in #5212, we'd like to improve the organisation and naming of the controllers and their methods. After that ADR has been implemented we will have the following controller structure:

SmartAnswerController

  • index (Index page)
  • show (Start page)
  • visualise (Visualisation page)

FlowController

  • start (Redirect to the first flow question)
  • show (Question and outcome pages)
  • update (Redirect to next question or outcome)
  • destroy (Clear responses and redirect to start page)

@kevindew raised that it is confusing having "SmartAnswersController" and "FlowController", as it unclear what each is responsible for. This may be further exacerbated by the concepts "Smart Answer" and "Flow" not having been strictly defined or existing conflicting usage.

The other change proposed was moving the visualise method from SmartAnswerController to the FlowController, due to it being publicly available in production environment. Start page and index pages are not rendered or accessible from Smart Answers in production. However, it's not clear whether visualise is a feature we support for public use, outside its primary use as a tool to help flow development.

@kevindew
Copy link
Member

kevindew commented Mar 8, 2021

Thanks for opening this @theseanything 👍

The area that I think is most surprising is the start actions in the FlowController. I find it rather surprising that this doesn't return a start page as that's such a closely related concept.

The other actions on the FlowController also seem to be a little surprising as they are named the same as Rails resources action names but don't perform in the same way. Convention would dictate that the they all have a resource id and used GET/PUT/DELETE methods respectively).

I'd wonder if we can get the action naming closer to the domain of the flow? We could probably use node instead of show since it seems to always refer to a node, answer or next might cover update since it's always about providing an answer and the URL has /next in it. destroy could be exit or leave to reflect the leaving process. I imagine index could replace show since that seems conventionally Rails for the root of a resource.

I'd imagine since we do publicly offer /visualise and it's probably a can of worms to deal with "do we want to do it"? it's probably simplest to just move that to FlowController and defer the wider considerations.

I'd wonder with the remaining SmartAnswerController whether we'd want to separate that into a DevelopmentController for the index page, that is purely development? Then that'd leave just start page which could go in a StartPageController, I guess a single action of index would make sense?

@theseanything
Copy link
Contributor Author

The area that I think is most surprising is the start actions in the FlowController. I find it rather surprising that this doesn't return a start page as that's such a closely related concept.

I think the original intention for the /<flow-name>/start was to have a permalink to the first node in the flow. I think the need arose when start page were rendered separately by Frontend and the fixed path allowed us update the first node without having to re-publish the start page. This potentially is no longer an issue now everything is rendered by Smart Answers.

The start action also seems to duplicate behaviour if the destroy_session action (difference being users go to the first question instead of the start page). Could we simplify things, and have users' always return back to the start page to start again - so we?

Then that'd leave just start page which could go in a StartPageController, I guess a single action of index would make sense?

So now that start pages are rendered by Smart Answers. I wonder if start pages should be treated like first class nodes, rather than having special logic to handle them?

I'd wonder if we can get the action naming closer to the domain of the flow? We could probably use node instead of show since it seems to always refer to a node, answer or next might cover update since it's always about providing an answer and the URL has /next in it. destroy could be exit or leave to reflect the leaving process. I imagine index could replace show since that seems conventionally Rails for the root of a resource.

Yup - relating the action names back to domain model of Smart Answers seems sensible. Think node instead of show, and next instead of update makes sense. Not sure about exit or leave for destroy - user's aren't strictly "leaving" the flow if they go back to the start page. It behaviour seems to be more about clearing or erasing their existing responses. Maybe clear?

I'd imagine since we do publicly offer /visualise and it's probably a can of worms to deal with "do we want to do it"? it's probably simplest to just move that to FlowController and defer the wider considerations.

Yeah so my understanding of visualise is that it's primary use it to help people build or modify existing flows, not something intended for the wider public. So having under say a DevelopmentController, would help separate those concerns.

Potentially we could have the following?

FlowController

  • node (Render start, question and outcome nodes)
  • next (Redirect the next node)
  • clear (Clear responses and redirect to start page (or BBC weather if flag present))

DevelopmentController

  • index (Index page)
  • visualise (Visualisation page)

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

No branches or pull requests

2 participants