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

Feature/add asset from model view for #365 #406

Closed
wants to merge 4 commits into from
Closed

Feature/add asset from model view for #365 #406

wants to merge 4 commits into from

Conversation

svpernova09
Copy link
Contributor

In reference to #365

As I said on the issue, I'm not crazy about this implementation since it's copying over all the data instead of some. Awaiting feedback from @diwanicki before tweaking this. Likely needs to pass in JUST the data the feature requester was wanting to have preselected instead of everything.

@diwanicki
Copy link

I just looked over your changes, but I am confused - I don't see where a
call is made to pass info to create asset blade... I'm going to spin this
build up in my test VM and get back to you with some feedback.

Thanks for working on this :)

On Wed, Dec 3, 2014 at 8:33 AM, Svpernova09 notifications@github.com
wrote:

In reference to #365 #365

As I said on the issue, I'm not crazy about this implementation since it's
copying over all the data instead of some. Awaiting feedback from
@diwanicki https://github.com/diwanicki before tweaking this. Likely
needs to pass in JUST the data the feature requester was wanting to have

preselected instead of everything.

You can merge this Pull Request by running

git pull https://github.com/svpernova09/snipe-it feature/add-asset-from-model-view

Or view, comment on, or merge it at:

#406
Commit Summary

  • Add optional parameter for create/model route to pass along a model
    to copy values from.
  • Check the optional model_id parameter, if not null fetch that model
    and pass it along to the view
  • Add a link to the dropdown to create a new asset based on the
    current model
  • Adding back missing comment description

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#406.

@svpernova09
Copy link
Contributor Author

@diwanicki 182d221 is where I added the optional parameter to the route and 1596990 is where I added the link to the drop down to pass in the model id that is currently being viewed to the app/controllers/admin/ModelsController.php@getCreate() method

@diwanicki
Copy link

just ran your branch... this is not what I was looking for - this is
basically just a model clone. I was wanting to be able to add an actual
asset.

The dropdown link (renamed 'New Asset') should take me to a new asset form,
with the model already filled out:

Clicking this -


Would take the user here:

Apologies if I wasn't clear

On Wed, Dec 3, 2014 at 11:00 AM, Svpernova09 notifications@github.com
wrote:

@diwanicki https://github.com/diwanicki 182d221
182d221
is where I added the optional parameter to the route and 1596990
1596990
is where I added the link to the drop down to pass in the model id that is
currently being viewed to the
app/controllers/admin/ModelsController.php@getCreate() method


Reply to this email directly or view it on GitHub
#406 (comment).

@svpernova09
Copy link
Contributor Author

Right, this is a clone.

Looks like your images didn't come through. Try throwing them on imgur?

@diwanicki
Copy link

I replied via gmail, should see them now:
screen shot 2014-12-03 at 11 15 00 am

opens this

screen shot 2014-12-03 at 11 16 32 am

@svpernova09
Copy link
Contributor Author

Gotcha. I see what you're wanting now.

@svpernova09
Copy link
Contributor Author

I was headed in the wrong direction. Will close this and re-open with a cleaner PR. Thanks for the quick feedback.

@svpernova09 svpernova09 closed this Dec 3, 2014
@svpernova09 svpernova09 deleted the feature/add-asset-from-model-view branch December 3, 2014 19:29
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