Conversation
|
Forgot to check out a new branch before committing changes for #492. So now this PR includes the work for public DMP export with multiple phases |
app/controllers/roles_controller.rb
Outdated
| redirect_to controller: 'plans', action: 'share', id: @role.plan.id | ||
| end | ||
|
|
||
| def archive |
There was a problem hiding this comment.
I'm not 100% on the archive action name for this. Perhaps deactivate is a bit clearer on what it does - "remove" is certainly not accurate (even though it eventually refers to "remove plan" action)
There was a problem hiding this comment.
perhaps "hide" or "disable" would be more appropriate? Also add a comment that summarizes what its doing: "Removes the plan from the user's list in the UI. The user's relationship to the plan does not change within the database"
There was a problem hiding this comment.
I'll update the name tomorrow morning.
Good catch on the lack of comment, I'll add a small block describing the functionality
| @role.plan.owned_by?(@user.id) | ||
| end | ||
|
|
||
| def archive? |
There was a problem hiding this comment.
Again not 100% sold on the name
| <!-- if the user has projects --> | ||
| <p> | ||
| <% if @plans.count > 0 %> | ||
| <% if @plans.length > 0 %> |
config/routes.rb
Outdated
| resources :roles, only: [:create, :update, :destroy] | ||
| resources :roles, only: [:create, :update, :destroy] do | ||
| member do | ||
| put :archive |
There was a problem hiding this comment.
Same comment about action name.
app/controllers/roles_controller.rb
Outdated
| redirect_to controller: 'plans', action: 'share', id: @role.plan.id | ||
| end | ||
|
|
||
| def archive |
There was a problem hiding this comment.
perhaps "hide" or "disable" would be more appropriate? Also add a comment that summarizes what its doing: "Removes the plan from the user's list in the UI. The user's relationship to the plan does not change within the database"
| end | ||
|
|
||
| def set_defaults | ||
| self.active = true if self.new_record? |
There was a problem hiding this comment.
nice. I have been using self.id.nil?. I like new_record? better and will start using it
There was a problem hiding this comment.
Thanks. stole it from some guy's blog on best practices for setting defaults in rails
|
|
||
| def show? | ||
| @plan.readable_by?(@user.id) | ||
| @plan.readable_by?(@user.id) && Role.find_by(user_id: @user.id, plan_id: @plan.id).active |
There was a problem hiding this comment.
It makes sense to keep this active check out of the readable_by? method. Is the check necessary though? These plans are filtered out of the user's dashboard, so the likelihood of them hitting these policy checks is slim. Even if they did via a bookmark or something, they should technically be able to access them.
This is maybe a discussion for a separate ticket though. I could see use cases where we would send them to these endpoints to view the plan and restore/unhide them if we decide to offer that option.
There was a problem hiding this comment.
The check is probably not needed, but If a user removes themself from a plan, and other users cant see that they can access the plan, I like ensuring that behavior in code.
|
Just realized while reading over your comments that I didn't filter the 'share plan' page to remove the roles which are not active. I'll add that, make the recommended changes, and update the PR. |
|
looks good @xsrust |
3 participants
Addresses #242, with implementation as detailed in the ticket
Note: this code will require a db migration to add the 'active' flag to the roles table