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

feat: Static Templates #567

Merged
merged 27 commits into from
Nov 26, 2024
Merged

feat: Static Templates #567

merged 27 commits into from
Nov 26, 2024

Conversation

cmaddox5
Copy link
Contributor

This PR adds a static templates feature. We will keep a list of static templates in priv/config/static_templates.json. When a message is created using a template:

  • visual and phonetic text cannot change
  • priority and interval default to a value determined by the type of template selected
    • These values can change after defaults are applied
  • message cannot be associated with an alert

I have also added an archived field to each static template in order to prevent IDs from being reused by mistake. If a template is deleted and the ID is used again later, any message linked to the deleted template would then be incorrectly linked to the new one. This is unlikely to happen, but wanted to prevent it nonetheless.

  • For features with a design/UX component, deployed branch to dev-green and let product know it's ready for review.

else
conn
|> put_status(404)
|> json(%{error: "not_found"})
end
end

def static_templates(conn, _) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies if this was already discussed, but do we need to serve these from the backend? I was imagining just compiling the template text into the frontend bundle and accessing it statically, since that's the only place we need it.

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 don't think server vs. client was ever discussed, but that's an interesting idea. It didn't click until now that the server has no reason to care about the contents of the file. That should simplify some things so I can give that a shot and see how we feel.

@cmaddox5 cmaddox5 marked this pull request as ready for review November 21, 2024 19:50
@cmaddox5 cmaddox5 requested a review from a team as a code owner November 21, 2024 19:50
Copy link
Collaborator

@panentheos panentheos left a comment

Choose a reason for hiding this comment

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

See comments, then looks good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there are two copies of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Forgot to delete the test fixture as well.

}
};

let content;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Since we're just returning the content immediately, we could put return statements inside each if and avoid a let here.

onCancel={() => setPage(Page.MAIN)}
onSelect={(template) => {
setSelectedTemplate(template);
setVisualText(template.audio_text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

Suggested change
setVisualText(template.audio_text);
setVisualText(template.visual_text);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, thank you.

@cmaddox5 cmaddox5 merged commit 955e634 into main Nov 26, 2024
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/static-templates branch November 26, 2024 13:59
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