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

Member only see listed rooms #60

Merged
merged 5 commits into from
Aug 3, 2020
Merged

Conversation

user512
Copy link
Contributor

@user512 user512 commented Aug 2, 2020

Part of #39
Hold off from implementing accessable_by because we now treat every guest as members
Screen Shot 2020-08-02 at 10 32 22 AM

@user512 user512 requested a review from zspencer August 2, 2020 17:32
Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

Looks good! I don't see any tests tho; is that intentional? Maybe just one for the Room model that lays out that unlisted rooms are not visible?

@@ -37,6 +37,8 @@ GEM
erubi (~> 1.4)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.1, >= 1.2.0)
active_record_union (1.3.0)
Copy link
Member

Choose a reason for hiding this comment

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

The README indicates that for folks using PostgreSQL, there is a more potent toolkit called ActiveRecordExtended.

This may give us more super-powers for the future; thoughts on using that instead of the one that just adds Union?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, didn't know about this tool.

@@ -9,7 +9,7 @@

<div class="p-6">
<ul class="mt-3 grid grid-cols-1 gap-5 sm:gap-6 sm:grid-cols-2 lg:grid-cols-4">
<% @workspace.rooms.each do |room| %>
<% @workspace.rooms.accessable_by.each do |room| %>
Copy link
Member

Choose a reason for hiding this comment

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

I think we still want to pass in current_user even if we're not using it for anything and it's a nil value. That way when we do start implementing users we don't have to find and update usages of accessable by.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but I believe we don't have current_user yet.

Copy link
Member

Choose a reason for hiding this comment

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

We could add a helper_method def current_user; nil; end to ApplicationController as a seam if we want.

user512 added 2 commits August 2, 2020 14:38
* Hold off from implementing accessable_by because we now treat every guest as members
@user512 user512 force-pushed the member-only-see-listed-room branch from 38dbfa1 to 3f68d60 Compare August 2, 2020 21:39
zspencer added a commit that referenced this pull request Aug 3, 2020
Now that Tom has removed Unlisted Rooms from the Workspace show page, I
have updated the feature file to indicate which scenarios work but are
missing functional step definitions.

See: #39
See: #60
@zspencer zspencer merged commit 6b5ef5d into development Aug 3, 2020
@zspencer zspencer deleted the member-only-see-listed-room branch August 3, 2020 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants