Support user-favorited workspaces in DB and model #165
Conversation
app/controllers/home_controller.rb
Outdated
| @@ -14,7 +14,7 @@ def explore | |||
| end | |||
|
|
|||
| def dashboard | |||
| @favorites = Workspace.accessible_by(current_ability, :update).favorites.order(updated_at: :desc).limit(3) | |||
| @favorites = current_user.favorite_workspaces.order(updated_at: :desc).limit(3) | |||
There was a problem hiding this comment.
Should we be checking if still accessible here? It's possible for a user to fav a workspace they have access to and then lose access to the workspace. 🤷♂️
| @@ -76,8 +76,36 @@ def destroy | |||
| end | |||
| end | |||
|
|
|||
| # POST /workspaces/1/favorite | |||
| def favorite | |||
| params.permit(:favorite, :id) | |||
There was a problem hiding this comment.
This is kinda ugly and for some reason (haven't looked into it yet), an unused and unpermitted :workspace param is coming through. not sure why, but we don't use it. rails machinery?
| if params[:favorite] | ||
| @workspace.favorited_by << current_user | ||
| else | ||
| @workspace.favorited_by.delete(current_user) | ||
| end |
|
|
||
| respond_to do |format| | ||
| if @workspace.save | ||
| format.html { redirect_to @workspace, notice: favorite_notice(params[:favorite]) } |
There was a problem hiding this comment.
This notice is never seen in practice because all the requests come through as JSON requests.
app/views/projects/show.html.erb
Outdated
| @@ -34,7 +34,7 @@ | |||
| <p class="fst-italic">There hasn't been any recent workspace activity in this project.</p> | |||
| <% end %> | |||
|
|
|||
| <% favorites = @project.workspaces.favorites.accessible_by(current_ability) %> | |||
| <% favorites = current_user.favorite_workspaces %> | |||
There was a problem hiding this comment.
Check accessibility again or nah?
There was a problem hiding this comment.
Not sure if this is relevant or not, but if a user favorited a workspace that they then lose access to, I think it would be fine to still keep track of the user's favorite status of that workspace. The user shouldn't see a favorited workspace if they don't have access to it, of course, but if they are later re-granted access to it, it does seem reasonable that upon re-access it is shown to that user as one of their favorite workspaces (since it was before and they never said it wasn't).
This might already be how things work, or might not be possible with the way things are stored in the database, but I just wanted to offer that opinion on the general workings of things.
mjgiarlo
force-pushed
the
user-specific-favorites#141
branch
from
4aa3a12
to
a35f22e
Compare
| end | ||
|
|
||
| respond_to do |format| | ||
| format.html { redirect_to @workspace, notice: favorite_notice(params[:favorite]) } |
There was a problem hiding this comment.
In practice, no one ever sees this but just in case...
app/views/projects/show.html.erb
Outdated
| @@ -34,7 +34,7 @@ | |||
| <p class="fst-italic">There hasn't been any recent workspace activity in this project.</p> | |||
| <% end %> | |||
|
|
|||
| <% favorites = @project.workspaces.favorites.accessible_by(current_ability) %> | |||
| <% favorites = current_user.favorited_workspaces %> | |||
There was a problem hiding this comment.
Still check accessibility here or?
There was a problem hiding this comment.
Yes, I think so? it's possible to favorite a public workspace, and then the workspace owner might take it private again?
app/controllers/home_controller.rb
Outdated
| @@ -14,7 +14,7 @@ def explore | |||
| end | |||
|
|
|||
| def dashboard | |||
| @favorites = Workspace.accessible_by(current_ability, :update).favorites.order(updated_at: :desc).limit(3) | |||
| @favorites = current_user.favorited_workspaces.order(updated_at: :desc).limit(3) | |||
There was a problem hiding this comment.
On a previous commit, I think you asked if we should keep checking accessible workspaces here? Maybe @ggeisler has thoughts about whether to:
- just hide non-accessible workspaces
- show... something.. to let the user know they no longer have access
- offer a broken link?
There was a problem hiding this comment.
Yeah, my earlier comment was ideally we should continue to track that a user favorited a given workspace, even if it has been made inaccessible to them (in case it later becomes accessible).
But definitely a second part of that is do we alert the user to the situation, and if so, how?
I don't know. Ideally, I guess some sort of notification might be helpful. But I'm also not sure it's a frequent or significant enough case to worry much about at this point. I'd suggest we go with "just hide non-accessible workspaces" and I can create a design-needed ticket to remember to think about this further and maybe do something more sophisticated in the future?
There was a problem hiding this comment.
OK! Will see to this later in the PM. Thanks, all.
mjgiarlo
force-pushed
the
user-specific-favorites#141
branch
from
a35f22e
to
b711fd4
Compare
Connects to #141
TODO: