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

[Iteration 1]Merge spring 24 main to snp-cloud main #356

Merged
merged 132 commits into from
Mar 12, 2024

Conversation

warrenlet
Copy link
Contributor

@warrenlet warrenlet commented Mar 8, 2024

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the upstream master branch.
  • The tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works(if appropriate).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves/which issues does this fix?:

#344

Changes proposed in this pull request:

  • changes attribute 'submission instructions' to 'submission template' for event_types model. Changed corresponding view, controller, model, and testing code.
  • Makes it such that no user (admins, non-admins) can see these toggles. An admin is still able to change these settings by going to administration->users->edit. The button for admin has also been updated from a checkbox to a toggle switch.
  • Update the INSTALL_SNAPCON.md to provide instruction to the situation of fork safty issues from MacOS

template_name: template_name)
# if email subject is empty, use custom template
if @ticket_purchase.ticket.email_subject.empty? && !@ticket_purchase.ticket.email_body.empty?
@ticket_purchase.ticket.email_body = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_body)
Copy link

@fp456 fp456 Mar 8, 2024

Choose a reason for hiding this comment

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

The conditional statements is very self explanatory so I think this comment can be deleted

Copy link

@fp456 fp456 left a comment

Choose a reason for hiding this comment

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

Hi! Good overall, just some clean up to do and data to be updated about the members of the team

if @ticket_purchase.ticket.email_subject.empty? && !@ticket_purchase.ticket.email_body.empty?
@ticket_purchase.ticket.email_body = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_body)
mail(subject: "#{@conference.title} | Ticket Confirmation and PDF!", template_name: 'custom_ticket_confirmation_template')
# if email body is empty, use default template with subject
Copy link

Choose a reason for hiding this comment

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

The conditional statements is very self explanatory so I think this comment can be deleted

elsif !@ticket_purchase.ticket.email_subject.empty? && @ticket_purchase.ticket.email_body.empty?
@ticket_purchase.ticket.email_subject = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_subject)
mail(subject: @ticket_purchase.ticket.email_subject, template_name: 'ticket_confirmation_template')
# if both exist, use custom
Copy link

Choose a reason for hiding this comment

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

The conditional statements is very self explanatory so I think this comment can be deleted

@ticket_purchase.ticket.email_body = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_body)
@ticket_purchase.ticket.email_subject = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_subject)
mail(subject: @ticket_purchase.ticket.email_subject, template_name: 'custom_ticket_confirmation_template')
# if both empty, use default
Copy link

Choose a reason for hiding this comment

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

The conditional statements is very self explanatory so I think this comment can be deleted

@@ -0,0 +1,32 @@
project:
Copy link

Choose a reason for hiding this comment

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

Please update with your team information

it 'add a currency conversion', feature: true do
visit admin_conference_currency_conversions_path(conference.short_title)
click_link 'Add Currency Conversion'

Copy link

Choose a reason for hiding this comment

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

you can remove this empty line

fill_in 'currency_conversion_from_currency', with: 'USD'
fill_in 'currency_conversion_to_currency', with: 'EUR'
fill_in 'currency_conversion_rate', with: '0.89'

Copy link

Choose a reason for hiding this comment

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

you can remove this empty line

Copy link

@fp456 fp456 left a comment

Choose a reason for hiding this comment

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

just some empty lines, tiny stuff but again overall looks good :)

Copy link
Member

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

Really minor things, but I will merge and might make a tidy-ing PR myself then deploy soon.

@@ -1,4 +1,6 @@
version: "2"
Copy link
Member

Choose a reason for hiding this comment

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

It's likely worth noting that this is now out of sync with the local config (I think...) but that's probably OK.

Comment on lines +52 to +53

# Only allow a list of trusted parameters through.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Only allow a list of trusted parameters through.

Comment on lines +40 to +41
# if email subject is empty, use custom template
if @ticket_purchase.ticket.email_subject.empty? && !@ticket_purchase.ticket.email_body.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# if email subject is empty, use custom template
if @ticket_purchase.ticket.email_subject.empty? && !@ticket_purchase.ticket.email_body.empty?
if @ticket_purchase.ticket.email_subject.empty? && @ticket_purchase.ticket.email_body.present?

Use .present? which is a little more readable than ! empty

if @ticket_purchase.ticket.email_subject.empty? && !@ticket_purchase.ticket.email_body.empty?
@ticket_purchase.ticket.email_body = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_body)
mail(subject: "#{@conference.title} | Ticket Confirmation and PDF!", template_name: 'custom_ticket_confirmation_template')
# if email body is empty, use default template with subject
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# if email body is empty, use default template with subject

@ticket_purchase.ticket.email_body = @ticket_purchase.generate_confirmation_mail(@ticket_purchase.ticket.email_body)
mail(subject: "#{@conference.title} | Ticket Confirmation and PDF!", template_name: 'custom_ticket_confirmation_template')
# if email body is empty, use default template with subject
elsif !@ticket_purchase.ticket.email_subject.empty? && @ticket_purchase.ticket.email_body.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elsif !@ticket_purchase.ticket.email_subject.empty? && @ticket_purchase.ticket.email_body.empty?
elsif @ticket_purchase.ticket.email_subject.present? && @ticket_purchase.ticket.email_body.empty?

spec/features/currency_conversions_spec.rb Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
<%= @ticket_purchase.ticket.email_body %>
Copy link
Member

Choose a reason for hiding this comment

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

Ideally there should be a newline.

@@ -32,11 +32,28 @@ def ticket_confirmation_mail(ticket_purchase)
attachments["ticket_for_#{@conference.short_title}_#{physical_ticket.id}.pdf"] = pdf.render
end

template_name = 'ticket_confirmation_template'
template_name = 'young_thinkers_ticket_confirmation_template' if @ticket_purchase.ticket_id == YTLF_TICKET_ID
if @ticket_purchase.ticket_id == YTLF_TICKET_ID
Copy link
Member

Choose a reason for hiding this comment

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

For the future, I think we can delete this code path once custom tickets are merged in

@cycomachead cycomachead merged commit 12f95c2 into snap-cloud:main Mar 12, 2024
8 of 10 checks passed
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.

8 participants