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

Added the ability to clone modpacks entirely. #470

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

EntranceJew
Copy link
Contributor

Addresses #71

  • Added a link in the sidebar to clone a modpack.
  • Cloning to an existing modpack will overwrite it with the source's packs.
  • Cloning to a nonexistent pack will create it.

I'm opening this up to talk about some of the concerns mentioned here and how they should be handled:

  • There are no permissions associated with cloning.
  • Destination list is loaded via JS therefore the modpacks have to be public even if the user has access to them. (Am I missing something here?)
  • No modpack permissions are considered before handling cloning to/from anything.

- Added a link in the sidebar to clone a modpack.
- Cloning to an existing modpack will overwrite it with the source's packs.
- Cloning to a nonexistent pack will create it.

Some concerns:
- There are no permissions associated with cloning.
- Destination list is loaded via JS therefore the modpacks have to be public even if the user has access to them.
- No modpack permissions are considered before handling cloning to/from anything.
@GenPage
Copy link
Contributor

GenPage commented Sep 29, 2015

Solid implementation! A couple things to note:

  • You should verify/sanitize the inputs before performing a Eloquent lookup. For Example:
$source = Modpack::where('slug', '=', Input::get('source'))->first();
$destination = Modpack::where('slug', '=', Input::get('destination'))->first();

vs.

$rules = array(
            'source' => 'required',
            'destination' => 'required'
         );

$messages = array(
            'source_required' => 'You must enter a modpack slug.',
            'destination_required' => 'You must enter a modpack slug'
         );

$validation = Validator::make(Input::all(), $rules, $messages);
    if ($validation->fails())
        return Redirect::to('modpack/create')->withErrors($validation->messages());

$source = Modpack::where('slug', '=', Input::get('source'))->first();
$destination = Modpack::where('slug', '=', Input::get('destination'))->first();

Yes its more code, but it prevents issues.

  • As for destination JS functions and loading from the API, thats a bit more tricky. Ideally instead of doing an AJAX call, you should populate both dropdowns with all modpacks on page load. There is no real need to update the destinations dropdown against the API, everytime the selected source modpack changes.

I'll work on implementing the permissions. If you could add some unit tests as well, that would be awesome! 👍

- Clone inputs are validated and required.
- Fixed issue where min_memory wasn't actually being copied.
- Fixed issue where cloned builds weren't also cloning modversions.
- Removed using AJAX to get the list of destinations.
- Added validation condition to ensure cloning to the same modpack doesn't happen.
- Removed the ability to create entries for the source selector. (This was meant for debugging only.)
@silent-mobious
Copy link

Sorry about necro-ing an old uncommitted commit. It fixes something I've been desperately wanting in Solder with the cloning, however unless I'm missing something it doesn't actually clone the mods associated with the builds of the source pack. I've got the new pack and builds, but no mod contents post-clone. Is there a permission issues I'm hitting? I'm not sure this is acting as intended, since the whole point of cloning it seems would be to migrate the mods as well.

Any ideas?

solder-1

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.

3 participants