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

Setup improvements #22

Merged
merged 45 commits into from
Dec 18, 2023
Merged

Setup improvements #22

merged 45 commits into from
Dec 18, 2023

Conversation

TheSlimvReal
Copy link
Contributor

@TheSlimvReal TheSlimvReal commented Oct 20, 2023

  • Keycloak is required for new deployments
  • Merge compose files and use profiles to enable/disable backend
  • Automatically fetch latest app version from GitHub
  • Args can also be passed through command line
  • A Keycloak user is directly generated and a invitation mail is send out
  • Add instructions to infinitely listen to pipe and how to stop it
  • Verify that no other commands can be run on server through pipe
  • Add default templates for common scenarios
  • Write setup script logs to
  • Add assets to deployment
  • Default language

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Although I can't claim I re-checked every http request parameter or shell script arg mapping in the last detail.

From functional testing in combination with the deployer-backend + elementor plugin:

  • What happens if using an existing sitename? Currently no feedback in the website form to users
  • Email after signup:
    • subject + headline should contain “Aam Digital”
    • if possible, text should include a sentence about the new system being ready (not only talking about confirming email address)
  • Keycloak display name would better be “Aam Digital - PROJECT”
  • App:
    • no SiteSettings doc was available (clicking on it in menu also fails), but I do see it here, so maybe I wasn't testing on the most recent code
    • we should already set the name in SiteSettings (for the toolbar) to what the user selected (on /self-signup), I think it will say "Demo" otherwise.

otherwise functionality looks really nice!

deployer/pipe-listener.sh Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@@ -0,0 +1,3 @@
#!/bin/bash
# shellcheck disable=SC2046
while true; do (cd /var/docker/setup; ./interactive_setup.sh $(cat deployer/arg-pipe) &> deployer/deploy-log.txt); done
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get our security expert to try and break this via the deployer-backend? I haven't invested time into possible hacks here yet.

@TheSlimvReal
Copy link
Contributor Author

TheSlimvReal commented Dec 12, 2023

@sleidig thanks for the extensive review.

  • Thanks for testing with the site name. It now shows a error message in the UI if this is the case. The UI also waits not until the deployment is done. See Error handling deployer-backend#4
  • Keycloak display name is Aam Digital - PROJECT
  • App site settings are present an name is Aam Digital (getting a custom site name in there is quite difficult)
  • Headline contains Aam Digital (due to display name)
  • Subject contains Aam Digital -> Is that required as we have Aam Digital in the sender + in the message content? Other mails I get rarely have their name in the subject
  • Adjust text -> quite difficult as Keycloak only comes with a limited set of actions and adding a required action is quite some work

@TheSlimvReal TheSlimvReal requested a review from sleidig December 12, 2023 21:56
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates :-)

From further testing:

  • site name accepts special characters (although it refuses space now) and then seems to break
  • with the "error" in the end, it was loading a long time on the form. Can we display a message "Creating your system, this may take a few minutes ..." below the button once it's clicked?
  • for the emails, I understand that customizing is too difficult. How about changing the email subject to "Your Aam Digital account" - which would be reasonable both for the first mail after registration as well as for password resets?

@TheSlimvReal
Copy link
Contributor Author

@sleidig I improved the error handling and updated the email subject. For the submission I did not find a way to add some kind of progress spinner. We can try with a text on the submit button that indicates that it might take a little longer. Or if you have an idea how to improve this in elementor feel free to give it a try. There might be some fancy plugins out there just for this.

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works really well now!

I have tweak the form in elementor a bit with styles and the German messages: https://aam-digital.com/codo-setup/

The new keycloak email subject was not deployed yet, I think. But overall great and good to merge :-)

@TheSlimvReal TheSlimvReal merged commit 2941242 into master Dec 18, 2023
@TheSlimvReal TheSlimvReal deleted the improved_setup branch December 18, 2023 17:43
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

Successfully merging this pull request may close these issues.

2 participants