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

Modifying pageflow for inclusion on complex existing apps #636

Closed
DaKaZ opened this issue Oct 4, 2016 · 5 comments
Closed

Modifying pageflow for inclusion on complex existing apps #636

DaKaZ opened this issue Oct 4, 2016 · 5 comments

Comments

@DaKaZ
Copy link

DaKaZ commented Oct 4, 2016

Hello pageflow team!

We stumbled upon Pageflow a few weeks ago and fell in love :) So, like all good developers we went to run the Pageflow engine on our Rails 4.2.6 app. Its a pretty good size rails backend app with 3 years of history. We have encountered a number of items which need to be discussed with the core developers, that is the purpose of this issue so that we can candidly discuss how to best proceed so that the PR works for everyone.

As an example of these issues, we have two devise models: Users (our customers) and AdminUsers (our active admin administrators). Thus... we have set out to create a pull request that will fix the issues we have been discovering while integrating Pageflow with our app so that others don't face the same problem.

Currently, one of the major issues we are having is with paperclip integration and S3.

First, overriding the paperclip configuration specific to Pageflow feels like a violation of what we SHOULD be doing. It seems to us the Pageflow should operate with any file storage that paperclip provides, be that S3 or the local file system. In looking into this deeper, we found an even larger issue: the background processing of files breaks when the background processor is not on the local machine. For example, we have 2 front end application boxes and 2 helper boxes which run delayed_job. The front end boxes do not run delayed_job. In addition, we think that pageflow should be using ActiveJob and not require resque so that existing apps can use whatever job processor they need. We have not inspected the code enough to know if resque is the processor that will work, but if it is we could use self.queue_adapter = :resque to force resque for the pre-processing jobs.

So the two questions at hand are:

  1. Is there any reason why we should override paperclip's configuration in the pageflow initializer and not just use the system default?

  2. Is there a hard requirement for resque as the background processor or should we allow for any background process (note: we'll still have to ensure the job execution ON the machine that accepted the upload or that the processing machine has access to that file).

We look forward to your thoughts.

@tf
Copy link
Member

tf commented Oct 5, 2016

Happy to hear that you like Pageflow!

Let me address the mentioned issues one by one:

  • At the moment Pageflow's authorization system assumes the existence of a User model. I could see there being a Pageflow.config.user_model option to swap in an AdminUser instead. There will probably be some questions to answer around how to set up ActiveRecord associations for this configurable user model in classes like Pageflow::Membership and how to extend the test suite to at least partly cover that case. One also would have to decide how to proceed with the user admin provided by Pageflow. All in all, I suppose it won't be an easy change, but should be doable.
  • Regarding Paperclip configuration, the current config options act more or less as a release valve to provide some freedom in configuration. In theory all Paperclip adapters should be supported. On the one hand, there are some restrictions, though, when transferring files to Zencoder since the current implementation only supports providing files to Zencoder via S3. On the other hand there have been some bad naming choices in the past, specifically referencing "S3" in the name of attachments (i.e. VideoFile#attachment_on_s3) even though the configuration would allow to actually use non-S3 storage adapters. Please see Allow non S3 Storage for Image Files #54 and Make Zencoder optional #72 for further context.
  • With our own instances, we use a custom NFS setup to share files uploaded to one instance with the instance running the background workers. The file processing jobs handle cases where files have not yet been transferred to the instance. Looking back it probably would have been the better choice to use some form of direct S3 upload.
  • Regarding Resque, now that we are on Rails 4.2, I'd be happy to use ActiveJob to loosen the dependency. Apart from the generators which setup all some Resque infrastructure, I do not see any hard dependency on Resque as a background worker solution.

None of these issues is really pressing for us at the moment, so I don't expect we will be doing much work in any of these areas in the near future. Still I'd be happy to provide pointers and review code if you have time to work on some of these points. I think almost all could lead to major improvements of the code base.

@DaKaZ
Copy link
Author

DaKaZ commented Oct 5, 2016

Hi @tf,

Thanks - we are not necessarily asking for you assistance in the modifications, we just don't want stub any toes and make sure the work we do is inline with your current work/architectural goals.

Would you like on MASSIVE pull request or each one as a smaller component? For example, we have already completed the work required to change the devise model as well as a custom mount point (which required a lot of updates to the JS code).

I am feeling like it would be more manageable if we submitted PRs for each section BUT we have already completed several of the modifications and I would hate to try to issolate each of them. If you are curious, our current modified fork is here: https://github.com/rg34/pageflow

Just let us know what works best for you and we'll be good team players :) Thanks again for all the work you all have put into pageflow so far!

@tf
Copy link
Member

tf commented Oct 5, 2016

I definitely prefer individual PRs for the different aspects (mount point, user model etc.). As chance would have it, we merged a quite extensive change to the permissions system today. So I expect this to cause some conflicts with the work you have done.

Some initial thoughts after skimming through the commit:

  • I'm not really sure what the relation of current_pageflow_user and current_active_admin_user should be. At least inside admin resources, I can see no reasonable case where the two would differ. All usages of current_user inside ActiveAdmin resource so far assume they are talking about current_active_admin_user. Maybe we should be using that instead?
  • I am also not sure what role admins/pageflow/user.rb would play in such a scenario. In your commit the managed resource is still User, but with authorization inside of ActiveAdmin based on AdminUser it would not make sense to manage Pageflow::Memberships for User since those have to exist for the AdminUsers to make sure they can access the ActiveAdmin resources defined by Pageflow.
  • Inside of non-ActiveAdmin controllers current_pageflow_user might make sense, simply since current_active_admin_user is not defined there. Still, it would have to point to the same model, right?
  • Regarding the custom mount point: Changing file extensions to .js.erb is not an option since it breaks editor syntax highlighting, causes caching issues within the asset pipeline and leads to hard to read code. There already is a mechanism to pass configuration options to the Backbone application. Those are then accessible inside JS as pagflow.config. Since the collection urls can be built lazily, initialization time should not be an issue.

I hope this makes some sense. Maybe I'm not yet understanding the use case correctly, especially regarding the relation between the User and the AdminUser model.

Best
Tim

@tf
Copy link
Member

tf commented Sep 12, 2017

Hi @DaKaZ,

this issue has not seen any activity in quite some time. Is this still something you are interested in working on?

@DaKaZ
Copy link
Author

DaKaZ commented Sep 12, 2017

Sorry, it because too hard to try and make a PR from our work, you can close this.

@tf tf closed this as completed Sep 12, 2017
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

No branches or pull requests

2 participants