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

Project creation automations #73

Merged
merged 6 commits into from
May 3, 2024

Conversation

dave-shawley
Copy link
Contributor

@dave-shawley dave-shawley commented Apr 18, 2024

This PR replaces the client-side project creation automations with server-side automations. They were moved to the server-side to:

  1. facilitate adding automations without writing state-dependent client code
  2. ensure that chained automations are cleaned up when a failure occurs

I also removed the ability to set links and available environments during project creation. Many of the links are going to be moved into individual automations to ensure that they are maintained properly. We can add them back if necessary.

This expects to run against a server with AWeber-Imbi/imbi-api#73 applied.

I left the remnants of the previous implementation in place to minimize
the churn in this commit. The automations code has been moved completely
to the server side now. The /ui/available-automations API returns the
list of automations available for THIS user and the selected project
type. We simply display toggles and pass the selected automations back
to Imbi.

I also removed the ability to create links, select environments, and set
URLs from this screen. Most of the links are going to be set via
automations in the future since having incorrect links is a problem in
current usage. You can easily add them later and I expect that they are
more likely to be correct ;)
@dave-shawley dave-shawley marked this pull request as ready for review May 1, 2024 14:45
Comment on lines 91 to 97
result.errors.map((err) => {
err.path.map((field) => {
if (newValues[field] !== null) {
errors[field] = err.message
}
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are arrays, this should use forEach instead, map is generating an unnecessary list.

results.errors.forEach((err) => {
  err.path.forEach((field) => {
    if (newValues[field] !== null) {
      errors[field] = err.message
    }
  })
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good point, lemme fix that one.

Comment on lines 24 to 30
function Automation({ automationName, integrationName, automationSlug }) {
return {
integrationName,
automationName,
automationSlug
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure what the point of this function is since it just returns a copy of what it got as input. Is it so you can have the proptypes defined below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that was it ... I really wanted to use it in AutomationList.jsx but I never figured out how to make it work nicely. Lemme clean that half-step up as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're looking for

Automation = PropTypes.exact({
  automationName: PropTypes.string.isRequired,
  automationSlug: PropTypes.string.isRequired,
  integrationName: PropTypes.string.isRequired
})
...
AutomationList.propTypes = {
  automations: PropTypes.arrayOf(Automation).isRequired,
  selectedAutomations: PropTypes.arrayOf(string).isRequired,
  setSelectedAutomations: PropTypes.func.isRequired
}

Copy link
Contributor Author

@dave-shawley dave-shawley May 3, 2024

Choose a reason for hiding this comment

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

I ended up dropping this altogether since I couldn't come up with a good way to get WebStorm to pay attention to the requirement so at best it results in a runtime explosion in the console and empty toggles on the screen 😞

Maybe I really want typescript here .... if it works with react hooks (eg, useState) that is.

Comment on lines 59 to 65
data.map(({ automation_name, integration_name, automation_slug }) =>
Automation({
automationName: automation_name,
integrationName: integration_name,
automationSlug: automation_slug
})
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@in-op can you think of a cleaner way to translate from snake_case to camelCase? I know that there is probably a package for this but I can't bring myself to pull in something like https://www.npmjs.com/package/change-case for this one case... maybe I should just let things be inconsistent

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, just manual

I forgot that about this... it was a half-finished attempt at adding a
data class between Project.Create and AutomationList that never really
worked out.
@in-op in-op merged commit fa80af4 into AWeber-Imbi:main May 3, 2024
3 checks passed
@dave-shawley dave-shawley deleted the project-creation-automations branch June 26, 2024 18:14
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