Skip to content

fix: removing extra check for Terms and conditions acceptance#1274

Closed
wtrocki wants to merge 2 commits intomainfrom
terms-and-condtions
Closed

fix: removing extra check for Terms and conditions acceptance#1274
wtrocki wants to merge 2 commits intomainfrom
terms-and-condtions

Conversation

@wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented Nov 2, 2021

No description provided.

@craicoverflow
Copy link
Contributor

Is there a supporting issue with the reason for this change? If this is merged, the user will simply be given a 403 forbidden error with no steps guiding them to complete the T&Cs?

@wtrocki wtrocki added the blocked label Nov 3, 2021
@wtrocki
Copy link
Collaborator Author

wtrocki commented Nov 3, 2021

Please ignore for the moment

@wtrocki
Copy link
Collaborator Author

wtrocki commented Nov 3, 2021

Comming back to this. I wanted to see how we can detect this specific error from the server.
My understanding is that the only reason for extra check was that interactive mode for Kafka requires user to specify 3 values before they will get notified about lack of terms and conditions acceptance. Is that right?

With different services now offering different terms and conditions this becomes more and more convoluted.

@pmuir proposal was to keep different terms keys outside CLI which is great idea but considering how little value IMHO we getting from checking terms upfront for Kafka I think we might just remove it and provide better message from backend to support it.

Another idea would be to have API on the backend to verify if user can create Kafka instance - checking some elements like limits, terms and conditions etc. This is probably not going to happen due to work required in backend so close to milestone

@pmuir
Copy link
Collaborator

pmuir commented Nov 3, 2021

Yeah, I'm really unhappy about a backwards step in usability that is caused by stripping this out.

A meta api to check all conditions is possibly a good idea. Can you propose this @wtrocki

@craicoverflow
Copy link
Contributor

My understanding is that the only reason for extra check was that interactive mode for Kafka requires user to specify 3 values before they will get notified about lack of terms and conditions acceptance. Is that right?

Yes, but upon reading the discussions about this, we can actually be more flexible with the following flow:

  1. User submits command request to create Kafka
  2. If API returns KAFKAS-MGMT-12 error code (indicating terms not accepted) then the CLI will "wait" and allow the user to read and accept the T&Cs, where they can press enter to submit the creation request again, without having to restart the entire process.

This still is not possible currently without the AMS SDK integration, as the backend does not return the specific information about the T&Cs - i.e. the URL to them.

and provide better message from backend to support it.

We usually don't want to print out the error message from the API directly, however I think in this case it could be okay, so long as the messaging is clear and makes sense to the user. It would also mean we could proceed with the suggested flow above.

@pmuir
Copy link
Collaborator

pmuir commented Nov 3, 2021

My understanding is that the only reason for extra check was that interactive mode for Kafka requires user to specify 3 values before they will get notified about lack of terms and conditions acceptance. Is that right?

Yes, but upon reading the discussions about this, we can actually be more flexible with the following flow:

  1. User submits command request to create Kafka
  2. If API returns KAFKAS-MGMT-12 error code (indicating terms not accepted) then the CLI will "wait" and allow the user to read and accept the T&Cs, where they can press enter to submit the creation request again, without having to restart the entire process.

This works well for the direct invocation of the command with paramters, but for the wizard flow it's still much better to check the preconditions up front.

@craicoverflow
Copy link
Contributor

I was actually thinking about this in a wizard flow - we still have the values the user had submitted.

$ rhoas kafka create                      
? Name: enda-dev
? Cloud Provider: aws
? Cloud Region: eu-west-1

Please agree to the terms and conditions etc etc: https://terms-and-conditions.com.

Press enter to submit again...

it's still much better to check the preconditions up front.

Agreed, this solution was a middle ground to decoupling the CLI and T&Cs event code, but also saving the user from having to reenter their data, as would be the case if we simply removed the pre-check.

A metadata API or YAML file would mean the existing flow could stay.

@wtrocki
Copy link
Collaborator Author

wtrocki commented Nov 3, 2021

but for the wizard flow it's still much better to check the preconditions up front.

For me, this is not something I would see as a problem considering how much complexity of reading metadata for each service, updating it, documenting for developers etc.

Service registry creation is now broken so it will be good if we decide short and long term plan
#1275

For me short term plan would be to remove terms and condition checks and match errors returned to backend with predefined i18n messages containing specific terms URL per service. Any opinions?

@wtrocki
Copy link
Collaborator Author

wtrocki commented Nov 3, 2021

I really like what @craicoverflow is suggesting. It resolves UX problem and minimizes complexity at the same time so we can do it right now
Number of files changed in this PR speaks for itself :D

@pmuir
Copy link
Collaborator

pmuir commented Nov 3, 2021

I've already expressed my opinion. This is a dreadful approach.

@wtrocki
Copy link
Collaborator Author

wtrocki commented Nov 4, 2021

superseded by #1276

@wtrocki wtrocki closed this Nov 4, 2021
@wtrocki wtrocki deleted the terms-and-condtions branch November 4, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants