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.

Sign up for GitHub

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

Jump to bottom

Valuation comments #2403

Merged
merged 10 commits into from Jan 31, 2018
Merged

Valuation comments #2403

merged 10 commits into from Jan 31, 2018

Conversation

bertocq
Copy link
Collaborator

@bertocq bertocq commented Jan 30, 2018 •
edited

Where

What

  • We need a comment thread on the investment valuation pages to enable replies instead of using deprecated internal_comment textarea by multiple users. Only adminsn and valuators assigned to an investment can create thos comments.

  • Those comments should be internal only and not displayed to public at investment pages or apis (and that thread should not show public comments)

  • Voting or Flagging comments won't be needed

How

  • Added a valuation boolean attribute to Comment model, as well as an index de615a5

  • Filtered valuation comments out from public api comments 09a6174

  • Created a second comment relationship named valuations at Budget::Investment model, and scoped existing comments relationship to non-valuations so everything around it won't be affected. 43c6804

  • Added valuation comment thread to Budget Investment Valuation show & edit pages 3ca134c taking care that only objects with a valuations relation can get a CommentTree for those comment types

  • Allowed valuation comment creation at Budget Investment Valuation show & edit pages 5dd0ef0

  • Added a new comment_valuation ability to admins and valuators with the same restrictions as the current valuate ability has. c87351d including a validation at Comment model to prevent creation of valuation comments without the ability

  • Hidden voting buttons on comments at valuations thread 9c81979 as well as flagging actions d14548a

Screenshots

Public comments work as previously

screen shot 2018-01-31 at 02 08 47

Without being show at valuation page, neither valuation comments at public page

screen shot 2018-01-31 at 02 11 49

Notifications work for valuation comments

screen shot 2018-01-31 at 02 10 17

Test

Created a new Budget Investment Valuation feature under comments folder af8c9af

The scenarios have been copied from existing /spec/features/comments/budget_investments_spec.rb but altered to work with internal valuation comments.

Apart form the "common usage" there are scenarios to:

Create as well a :valuation trait for the comment factory that complies with abilities for valuators.

Increased existing Budget Investment feature spec to check no valuation internal comments appear on the public pages

Added scenario to comment model spec to check public api doesn't return valuation comments

Deployment

As usual

Warnings

The next PR will remove existing usage of Budget::Invesment#internal_comments and a rake task to migrate existing values to a first comment.

A third PR that creates Valuator Groups will fulfill the Valuator's names should be listed with their Alias and ValuatorGroup (if existent) #2362 requirement

This was referenced Jan 30, 2018
Closed
Merged
Merged
@bertocq bertocq force-pushed the valuation_comments branch 2 times, most recently from 44feee6 to 9ec0af2 Compare January 30, 2018 18:19
@bertocq
Add valuation boolean flag to Comment model
c84b2f0 
Why:

Budget Investment's valuators need to be able to comment on investments
without making those comments public. We need a way to clearly make a
distinction to avoid "leaking" internal valuation comments.

How:

Adding a boolean `valuation` attribute defaulted to false to the Comments
table, and index on it with concurrent algorithm as explained at
https://robots.thoughtbot.com/how-to-create-postgres-indexes-concurrently-in

The name `valuation` was chosen instead of `internal` because of the more
specific meaning as well as avoiding a collision with existing internal_comments
attribute on Budget::Investment model (soon to be deprecated & removed)
@bertocq
Filter internal valuation comments from public api
56fc5c9 
Why:

Internal valuation comments are only for admins and valuators,
not for the public view.

How:

Adding a `not_valuations` scope and use it at the `public_for_api` one
@bertocq
Add valuation comments relation at Budget Investment
767fd04 
Why:

Budget Investments already has an existing `comments` relation that is
on use. We need to keep that relation unaltered after adding the
internal valuation comments, that means scoping the relation to only
public comments (non valuation ones) so existing code using it will
remain working as expected.

A new second relation will be needed to explicitly ask for valuation
comments only where needed, again scoping to valuation comments.

How:

Adding a second `valuations` relationship and filtering on both
with the new `valuation` flag from Comment model.
@bertocq bertocq force-pushed the valuation_comments branch 2 times, most recently from d14548a to 635efe1 Compare January 30, 2018 20:50
@bertocq
Copy link
Collaborator Author

bertocq commented Jan 31, 2018

Different implementations where evaluated:

A- Copy&Paste the entire Comment feature as "Valuation". As easy and quick.. as but Brutal and Inneficient

B- Using a boolean "valuation" flag attribute at & a default scope at Comment model: Makes it easy to maintain existing comments feature intact, but has a huge caveat because many implicit "where/find" over Comment model would require the explicit use of unscoped making parts of the app that shouldn't be affected (like Notifications) aware of this changes. The ammount of collateral changes needed makes it inviable.

C- Using a boolean "valuation" flag and 2 explicit scopes (valuations & non_valuations) at Comment model: Lowest security level, we'd rather go for option A than make it so easy to create internal comment leaks as any dev not aware of this (or just by mistake) could request investment.comments without adding a scope... getting all of them.

D- Add another polymorphic relationshipvaluations between Comment & Budget::Investment models, in a similar way to existing comments relationship: The fact that has_many :valuations, as: :evaluable would require evaluable_type & evaluable_id columns, would break many existing code assuming a existing commentable (at Notifications for example). Its far better than 3 previous options, but again parts of the app that shouldn't be aware of this changes would require to do so (as well as having a ton of code to change explicitly).

E- Create another relationship at Budget::Investment using existing commentable polymorphism and the use of a valuation boolean flag from Comment model to scope that relationship and existing comments one: The solution implemented at this PR using

    has_many :comments, -> {where(valuation: false)}, as: :commentable, class_name: 'Comment'
    has_many :valuations, -> {where(valuation: true)}, as: :commentable, class_name: 'Comment'

To keep comments relationship unaltered across the app, and create a new explicit relationship just for valuation comments.

@bertocq
Show valuation comment thread @ Valuation show/edit
dff966d 
Why:

Budget Investment's valuators should be able to see internal valuation
comments thread at both show and edit views.

How:

At Valuation::BudgetInvestmentsController:
* Include CommentableActions to gain access to the entire feature, with
required resource_model & resource_name methods.
* Add the only possible order (oldest to newest)
* Load comments on both show & edit actions, passing `valuations` flag
to the CommentTree in order to only list those.

At CommentTree:
* Use `valuations` flag as instance variable to decide wich
comment threat to load: valuations (if relation exists) or comments.
@bertocq
Allow valuation internal comments to be created
149c813 
How:

Using a local variable at partials to set a hidden true/false value for
`valuation` parameter on the comment creation form.

Allowing that new param at the comment controller and using it when
building a new Comment.
@bertocq
Create valuation comments creation ability
26cf91c 
Why:

Only admins or valuators (for those investments they've assigned) can
create internal valuation comments on them.

How:

* Creating a new `comment_valuation` ability for admins and valuators in
the same manner the `valuate` ability works.

* Adding a validation at Comment model for those with `valuation` flag
active that checks if the author can make a valuation comment on the
commentable, as well as the respective active record error messages.
This will prevent comments from being created at a controller level as
well.

* Improving comment factory trait `valuation` to have an associated
investment, author that is a valuator and setting the valuator on the
valuators list of the investment
@bertocq
Hide comment voting at valuation comment threads
1c3924b
@bertocq
Add Budget Investment valuation comments feature spec
a4718f6
@bertocq
Disallow comment flagging when showing valuations
033c197
@bertocq
Avoid using comments_count counter_cache at Investment
4ec30a9 
As Budget::Investment has two relationships over commentable polymorphic
relationship, the counter_cache is counting the sum of both comments and
valuations.

We don't show valuations count anywhere, only the (public) comments so
we just use comments.count in this case
@bertocq bertocq mentioned this pull request Jan 31, 2018
Merged
@voodoorai2000
Copy link
Member

Exquisite 😌

@bertocq bertocq merged commit ec81f39 into master Jan 31, 2018
@bertocq bertocq mentioned this pull request Jan 31, 2018
Merged
@bertocq bertocq deleted the valuation_comments branch January 31, 2018 12:59
clairezed pushed a commit to CDJ11/CDJ that referenced this pull request Jun 26, 2018
@bertocq
Merge pull request consuldemocracy#2403 from consul/valuation_comments
d6710fc 
Valuation comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoorai2000 voodoorai2000 voodoorai2000 approved these changes

@MariaCheca MariaCheca Awaiting requested review from MariaCheca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bertocq @voodoorai2000