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

Implemented premium subscriber content #116

Merged
merged 4 commits into from Nov 19, 2014
Merged

Conversation

bdunogier
Copy link
Member

EZP-22324
Status: Tests fail because the demo data isn't updated yet, but they pass with the proper data.

Summary

Content from the Premium section will only be fully visible by members of the Premium subscribers group. Users who don't match this requirement will be shown a teasing page, showing a specific attribute's content, as well as a piece of text explaining premium membership.

Implementation

A custom location view provider displays a teasing template if the content is Premium (section id 7 (Premium)), and if the user doesn't have the subscriber role (id: 6).

Covered by behat scenarii.

Todo

  • Refactor role / premium content logic
  • Update demo data with: "Premium subscriber" role, "Premium" section, "Premium subscribers" role + assignment to group, John Doe Premium Subscriber user, Zachary Zoe Member user, Premium article (update URL in test once final)

@bdunogier bdunogier force-pushed the EZP-22324-premium_content branch 3 times, most recently from 3a61b9a to b7a63a2 Compare October 28, 2014 20:24
Scenario: As a subscribed User, I get the full content
Given I am logged in as "john.doe" with password "publish"
When I go to "/Premium-article"
Then I am on the "premium_article" page
Copy link
Contributor

Choose a reason for hiding this comment

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

For both lines above we need the actual name from PM, however unsure what identifier should ideally be.
@ezsystems/qa-team: Anyone up for suggesting something on how to identify it?

Choose a reason for hiding this comment

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

Then lets call it "Premium article" :-)
Actually it's straight forward and doesn't hurt.

@lolautruche
Copy link
Contributor

What about a custom content view provider?

@andrerom
Copy link
Contributor

Looks good from my side, but for BDD make sure someone from @ezsystems/qa-team has a look (ping)

And I see "This is the subscriber teaser" text
And I see "This article is only available to Premium subscribers" text
And I don't see "This summary will only be shown to subscribers" text
# And I see a login form
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing implementation? Plans for it?

Choose a reason for hiding this comment

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

Didn't we decide to skip the login form here to make it easier?
Great if we can make it, but nice to have

@andrerom
Copy link
Contributor

andrerom commented Nov 7, 2014

@bdunogier Will most likely setup a demo today for updating demo content, is this ready enough for @lserwatka to check this out on that demo setup so @rolandbenedetti can play around with this?

@bdunogier You should also rebase, to make sure the image fix is part of the checkout of this branch, but as demo will be using beta1 not the fix from JV for image filters, so:

git rebase a8eb4a6

// {
// return true;
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out? maybe remove or?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, remove.

@andrerom
Copy link
Contributor

andrerom commented Nov 7, 2014

Given it will not be possible to get this to pass before new demo content and new packages are created it might make sense to do code first approach, then content -> packages -> tests + adjustments

@rolandbenedetti
Copy link

Hello guys,

I am not sure totally about what’s happening there. Just keep in mind I need a little bit of time for the update.

@bdunogier
Copy link
Member Author

PR splitted, BDD tests moved to #120.

@bdunogier
Copy link
Member Author

Updated:

  • made the section and role id configurable using parameters.yml
  • refactored the ViewProvider into a ViewProvider and a SubscriptionChecker
  • added a short spec that (briefly) explains how it works

As far as I'm concerned (reviews aside of course), it is ready enough. It won't have any effect without applicable content.

@andrerom
Copy link
Contributor

andrerom commented Nov 7, 2014

Looks ok, but wasn't this supposed to be a content/location controller override?, if it was we would not have to introduce settings to configure which section id this is, and would not have to "pass a flag saying you want to ignore the custom view provider".

Reason for asking: this is not real security, no point imho in making a feature out of it on a low level with setting and stuff as long as it is not proper, it is better to just serve as an example.

@bdunogier
Copy link
Member Author

It's anything but proper. It is a bit cleaner than it was, but "proper" would be with semantic config and all :p

As long as the value is stored in a property, it doesn't make much sense not to inject it. Storing it as a direct parameter container is very cheap, and is already done for equivalent features in the DemoBundle.

Even if it was done with a Controller, you would either need to manually enable it. Do you think that it would have been easier to understand with a controller override for section X ? It is still a possibility, but it's still an extension point, just like view providers are. In that case, using a view provider to return the premium teaser view when applicable does make sense.

@bdunogier
Copy link
Member Author

Demo content updated with users in ezsystems/ezdemo#37.

@bdunogier bdunogier force-pushed the EZP-22324-premium_content branch 2 times, most recently from 91f00c8 to a439dca Compare November 19, 2014 07:25
A custom location view provider displays a teasing template
if the content is Premium (section id 7 (Premium)), and if the
user doesn't have the subscriber role (id: 6)

section_id and role_id are configured in default_settings.yml.
bdunogier added a commit that referenced this pull request Nov 19, 2014
Implemented premium subscriber content
@bdunogier bdunogier merged commit a7ba7b3 into master Nov 19, 2014
@bdunogier bdunogier deleted the EZP-22324-premium_content branch November 19, 2014 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants