-
Notifications
You must be signed in to change notification settings - Fork 125
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
Extend query interface #815
Conversation
aviav
commented
Jul 28, 2017
- Introduce AccountRoleQuery
- Introduce AccountMemberQuery
- Make AccountMemberQuery and EntryRoleQuery depend on ApplicationQuery
|
||
module Pageflow | ||
describe AccountRoleQuery do | ||
describe '.has_at_least_role?' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block has too many lines. [39/25]
require 'spec_helper' | ||
|
||
module Pageflow | ||
describe AccountRoleQuery do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block has too many lines. [41/25]
module Pageflow | ||
describe AccountMemberQuery do | ||
describe AccountMemberQuery::Scope do | ||
describe '.with_role_at_least' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block has too many lines. [38/25]
|
||
module Pageflow | ||
describe AccountMemberQuery do | ||
describe AccountMemberQuery::Scope do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block has too many lines. [40/25]
require 'spec_helper' | ||
|
||
module Pageflow | ||
describe AccountMemberQuery do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block has too many lines. [42/25]
class AccountMemberQuery < ApplicationQuery | ||
class Scope < Scope | ||
# Account whose members we scope | ||
# @return {Pageflow::Account} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the YARD doc they use []
for types. This is inconsistent in other files as well, but let's try to be consistent.
@@ -0,0 +1,57 @@ | |||
module Pageflow | |||
class AccountMemberQuery < ApplicationQuery | |||
class Scope < Scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works but does not read well. Let's be explicit and write ApplicationQuery::Scope
.
|
||
private | ||
|
||
# @api private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
stuff (in the Ruby sense) is @api private
by default.
class Scope | ||
protected | ||
|
||
# @api protected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@api protected
is not a thing so far. Let's make ApplicationQuery
@api private
completely since we do not expect anyone to inherit from it.
end | ||
|
||
def see_badge_belonging_to? | ||
(@account.entries & @user.entries).any? || | ||
@user.memberships.on_accounts.as_previewer_or_above.where(entity: @account).any? || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, FolderPolicy
could also benefit from using the query. Please check if some these _or_above
methods on Membership
are unused then and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Disregard the following two comments. I think adding a Scope is the way to go here:
Would this be okay in principle? Would we want to catch the error/throw something else?
def has_at_least_role?(role, on_as_many_accounts_as: 1)
case on_as_many_accounts_as
when 0
does_not_have_role?(role)
when 1
has_role_at_least_once?(role)
else
if on_as_many_accounts_as > 1
has_role_at_least?(role, on_as_many_accounts_as)
else
raise ArgumentError('Integer 0 or greater expected for '\
'on_as_many_accounts_as')
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We would need on_as_many_accounts_as: 2
for now, but doing it for 1
and 2
only seems excessively lazy)
Also, the other methods are supposed to be private, so the ambiguous naming is okay because of context and because long method names impede reading
Specific usage:
def show_account_selection_on?
(@user.admin? && Account.all.size > 1) ||
query.has_at_least_role?(:publisher, on_as_many_accounts_as: 2)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the Scope requires specs and maybe thought, so I will leave it for the next apparent opportunity. I agree, however, that we can probably use the extended query interface in other policies as well and thus improve readability, design and such
@@ -0,0 +1,57 @@ | |||
module Pageflow | |||
class AccountMemberQuery < ApplicationQuery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a one line doc comment to the query classes themselves.
1 similar comment
610dad7
to
59ae2ef
Compare
2017-11-07 [Compare changes](codevise/pageflow@12-0-stable...v12.1.0) - Include HLS in output presences of legacy files ([#872](codevise/pageflow#872), [#867](codevise/pageflow#867)) The database migration for Pageflow 12.0 which updates output presences of existing video files is missing the HLS variant. This causes HLS urls of existing video files to be rendered as `undefined`. To apply the fix, install migrations and migrate your database. This fix has previously been released as part of version 12.0.2. - Add Resque::Server to the generated routes ([#871](codevise/pageflow#871)) Mounting the Resque web server makes it easier to inspect background workers and restart jobs that have failed. See the issue description of [#856](codevise/pageflow#856) on how to add this to existing apps. - The theme configured on account level now only acts as a default for new entries. After enabling the `selectable_themes` feature, a theme selection dialog is available inside the editor from the "Title and Options > Appearance" tab. The dialog allows configuring the theme on a per revision basis. ([#781](codevise/pageflow#781), [#897](codevise/pageflow#897)) - Use page from url hash es landing page ([#832](codevise/pageflow#832)) - Do not record history when changing pages via scrolling ([#831](codevise/pageflow#831)) - Improve text tracks and info box display logic ([#826](codevise/pageflow#826)) - Bug fix: Fix order of public i18n fallback ([#883](codevise/pageflow#883)) - Bug fix: Prevent display of NaN duration in video controls ([#878](codevise/pageflow#878)) - Bug fix: Prevent 404 when share image has been deleted ([#816](codevise/pageflow#816)) - Use searchable select boxes in admin forms ([#888](codevise/pageflow#888)) - Remove sensitive data from active admin downloads ([#899](codevise/pageflow#899)) - Add config option to prevent multi account users ([#848](codevise/pageflow#848), [#868](codevise/pageflow#868)) - Add config options to restrict account manager permissions ([#849](codevise/pageflow#849)) - Fix N+1 query in account admin users tab ([#877](codevise/pageflow#877)) - Bug fix: Hide restore link and snapshot button for entry previewers ([#853](codevise/pageflow#853)) - Bug fix: Use copy of current_user in profile form ([#850](codevise/pageflow#850)) - Bug fix: Ensure new entry button is hidden for editors ([#847](codevise/pageflow#847)) - Bug fix: Show folder edit button only for publishers and above ([#838](codevise/pageflow#838)) - Bug fix: Allow :create entry only for publishers on accounts, not on entries ([#836](codevise/pageflow#836)) - Widget configuration ([#694](codevise/pageflow#694), [#809](codevise/pageflow#809)) - Bug fix: Ensure page order in editor preview stays up to date ([#898](codevise/pageflow#898)) - Bug fix: Switch off file meta data edit links when uploading ([#857](codevise/pageflow#857)) - Bug fix: Improve polling for HostedFile state ([#822](codevise/pageflow#822)) - Bug fix: Handle undefined page title. ([#763](codevise/pageflow#763)) - Use relative urls inside dash and hls playlists ([#842](codevise/pageflow#842)) - Use web audio api for volume fading if available ([#800](codevise/pageflow#800), [#863](codevise/pageflow#863)) - Add panorama_mask image file style ([#830](codevise/pageflow#830)) - Bug fix: Make url template generation more robust ([#876](codevise/pageflow#876)) - Themes can now be guarded by feature flags ([#765](codevise/pageflow#765)) - Extend EncodingConfirmation by public account attribute ([#817](codevise/pageflow#817)) - Extend query interface ([#815](codevise/pageflow#815)) - Accept options for accounts admin menu via config ([#811](codevise/pageflow#811)) - Add placeholder options to textareainputview ([#807](codevise/pageflow#807)) - Call hook on entry publication ([#806](codevise/pageflow#806)) - Add rake task and resque job to delete old auto snapshots ([#861](codevise/pageflow#861), [#882](codevise/pageflow#882)) - Generate a secure password in the seeds ([#775](codevise/pageflow#775)) - Allow specifying opacity of image variant logo ([#799](codevise/pageflow#799)) - Allow setting size of custom loading spinner background ([#798](codevise/pageflow#798)) - Add theme option to right align logo in desktop layout ([#797](codevise/pageflow#797)) - Add theme option to hide scroll indicator ([#790](codevise/pageflow#790)) - Make content text max width configurable ([#780](codevise/pageflow#780), [#804](codevise/pageflow#804)) - Import nginx-upload-module guide from wiki ([#814](codevise/pageflow#814), [#821](codevise/pageflow#821)) - Add documentation for versioning policy ([#866](codevise/pageflow#866)) - Fix small typos ([#787](codevise/pageflow#787)) - Precompile assets in Travis run ([#873](codevise/pageflow#873)) - Test workflow improvements ([#803](codevise/pageflow#803)) - Generate admins with account membership in specs ([#840](codevise/pageflow#840)) - Use rails 4.2.9 in tests ([#837](codevise/pageflow#837)) - Clean up memberships admin code ([#835](codevise/pageflow#835)) - Basic tests for UsersTab add user button ([#805](codevise/pageflow#805)) - Use codevise/teaspoon for logging backtraces on console ([#786](codevise/pageflow#786)) - Split vendored code from our own ([#885](codevise/pageflow#885)) - Introduce ApplicationRecord ([#889](codevise/pageflow#889)) - Move configuration into a concern ([#794](codevise/pageflow#794)) - Read the attribute instead of super ([#810](codevise/pageflow#810)) - Destroy widgets when parent subject is destroyed ([#834](codevise/pageflow#834)) - Fix dummy app generation on Travis for Rubygems 2.7 ([#893](codevise/pageflow#893)) - Update contents of gemspec ([#891](codevise/pageflow#891)) - Bug fix: Work around breaking change in resque_mailer 2.4.3 ([#894](codevise/pageflow#894)) See [12-0-stable](https://github.com/codevise/pageflow/blob/12-0-stable/CHANGELOG.md) branch for previous changes.