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

Moved shared published functionality to concern #27

Merged
merged 3 commits into from
Aug 8, 2019

Conversation

simaob
Copy link
Contributor

@simaob simaob commented Aug 8, 2019

This PR extracts the visibility status usage to a concern to be shared by different models.

I haven't done the same to the Admin pages under app/admin as I wasn't able to find a lot of guidance. I thought about extracting the scopes into a new concern for the admin bits, but not sure if it's worth it.

One example I found on Stack Overflow mentions using extend instead of include, because it seems that Active Admin doesn't support the ActiveSupport::Concern: https://stackoverflow.com/questions/27325605/dry-ing-up-activeadmin

Also found this word of warning: https://tmichel.github.io/2015/02/22/sharing-code-between-activeadmin-resources/

@simaob simaob requested a review from tsubik August 8, 2019 13:29
@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

Yeah, I know doing many things in AA is hard. Extending DSL is the best option here. There are some AA extensions already in the code I have a little cleanup in mind, maybe I could try to add that extension here.

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

Do it! :D

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

I'm also thinking if scope all should return archived or maybe the name all is not great here. Creating default scope without archived? not sure what other problems we will have after that.

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

We could not have "All" and just keep the specific ones, for clarity. Not sure when they'd need them all in a page.

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

Maybe default to Published.

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

I'm talking about model default_scope

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

That archived sounds for me like "soft-deleted". That should not be present in any select where you can link association.

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

We need All like this is a default scope that you get when you enter view, if we remove it then the only way to go back to see all would be page refresh.

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

oh right, yes, I get it. We should prevent archived objects from being linked to other objects, as they are archived. Makes sense to exclude archived from that.

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

Exactly, take a look at last commit, not sure what else could go into publishable "concern" for ActiveAdmin. I think loading extensions in this way is cleaner (to have them separately and load in initializer), what do you think?

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

That's quite nice and tidy. when I was looking at it, I couldn't see much else either. Potentially the sidebar bit for the show page?

@tsubik
Copy link
Contributor

tsubik commented Aug 8, 2019

Right, I thought that only one sidebar block is allowed but apparently we can add as many separate sidebar blocks as we want. Good idea, I will add it as a separate publishable_sidebar method.

Copy link
Contributor

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

We can do that scope for archive and testing in separate PR.

@simaob
Copy link
Contributor Author

simaob commented Aug 8, 2019

Thanks for the help! =)

@simaob simaob merged commit dde8ef1 into develop Aug 8, 2019
@simaob simaob deleted the chore/extract-visibility-status-to-concern branch August 8, 2019 16:15
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