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

Signup bcpage #2937

Closed
wants to merge 8 commits into from
Closed

Signup bcpage #2937

wants to merge 8 commits into from

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Apr 1, 2019

Related PRs/issues #2908

This is a (draft) PR to add signup functionality based on basket-managed newsletters to the bannered campaign page.

This code is not ready to land, but it is ready for a sanity check around how the signup form (from join.jsx) calls the API server's new /signups/<id> route, which is hanlded by signup_submission_view in networkapi.campaign.views.

There is a minimal amount of duplication in the views.py file, falling through to the function that petitions were already using to send data over to SQS, but with a new event type, newsletter_signup_data, rather than the existing crm_petition_data.

Checklist

Tests

  • Is the code I'm adding covered by tests?

Documentation:

  • Is my code documented?

@Pomax Pomax requested review from cadecairos and alanmoo April 1, 2019 22:32
@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-2937 April 1, 2019 22:32 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 1, 2019 22:34 Inactive
help_text='Choose an existing, or create a new, sign-up form'
)

panel_count = len(PrimaryPage.content_panels)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"put things in between other panels" - this adds the signup option between the existing cta option, and the body field.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 1, 2019 22:51 Inactive
@api_view(['POST'])
@parser_classes((JSONParser,))
@permission_classes((permissions.AllowAny,))
def signup_submission_view(request, pk):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is essentially a duplication of the petition code, but I wanted to keep the route view "its own thing" instead of making both signup and petition go to the same def and have its code unravel the actual url things came in on, which would make things far harder to read.

@Pomax
Copy link
Contributor Author

Pomax commented Apr 2, 2019

to be clear, @alanmoo @cadecairos this needs review, despite styling not being done.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 2, 2019 21:41 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Apr 2, 2019

test page with both CTA and signup component: https://foundation-mofostaging-pr-2937.herokuapp.com/en/signup-test-page/

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 3, 2019 18:12 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 3, 2019 18:14 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2937 April 3, 2019 18:15 Inactive
@@ -23,3 +23,45 @@
</div>
</div>
{% endblock %}

The following block has been borrowed from the templates/tags/cta/petition.html file,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this (or the above) need to be in some sort of comment tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as an extends template, anything that isn't inside a block gets entirely ignored. (based on "where would it even render it?" logic)

{% block cta %}
{% if page.cta != None or page.signup != None %}
<div class="cta
{% if page.specific.narrowed_page_content == False %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly out of scope, but I think this can be removed/made just one class- since if you're seeing the right column, the page content width is a fixed size anyway.

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'd say out of scope - it's a copy of what we already had so I'd rather leave it the same as much as possible until we tackle the cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the templating support wrapping block tags inside the if statement? Or can you call a block's super content manually? Just trying to reduce the WETness here.

Copy link
Contributor Author

@Pomax Pomax Apr 4, 2019

Choose a reason for hiding this comment

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

any toplevel block declaration means "I am overriding the content of this block, overriding everything the parent/ancestor has in that block: ..." and any block declaration inside of templating code means "I am defining a block here, that extending templates can assign their own content".

When overriding a block, leaving it empty will "remove" whatever the parent/ancestor said should happen, and when defining a block, it can similarly be left empty as a "placeholder" for extending templates to fill in as needed, or it can be given real content, that extending templates can then swap out for something else as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I wasn't sure if there was a secret manual call to super. Not a big deal, when you land this file a ticket to remember to DRY it out.

@Pomax Pomax mentioned this pull request Apr 4, 2019
1 task
Copy link
Contributor

@alanmoo alanmoo left a comment

Choose a reason for hiding this comment

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

This looks reasonable overall, just another comment or two that might be improvable. And obviously, the markup needs some love.

@Pomax
Copy link
Contributor Author

Pomax commented Apr 4, 2019

@mmmavis is helping with the markup because we'll need the signup component in the footer etc too.

@Pomax
Copy link
Contributor Author

Pomax commented Apr 23, 2019

superceded and landed

@Pomax Pomax closed this Apr 23, 2019
@Pomax Pomax deleted the signup-bcpage branch April 23, 2019 17:03
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.

3 participants