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

Devel #13

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Devel #13

wants to merge 8 commits into from

Conversation

majioa
Copy link
Contributor

@majioa majioa commented Oct 22, 2015

  • Forced column_name variable to be passed into partials.
  • boolean false value for custom display type isn't now treated as a blank.
  • add a new record for has_one/has_many associations
  • delete a record when csrf-token is used

@antpaw
Copy link
Owner

antpaw commented Oct 22, 2015

Hi thanks, i think you can't remove the compat layer of mootools because the form-datepicker plugin will not work

@majioa
Copy link
Contributor Author

majioa commented Oct 23, 2015

@antpaw that was compiled with a compat layer, just renamed.

@antpaw
Copy link
Owner

antpaw commented Oct 23, 2015

in this old file if i search for "compat" i get 69 results
in this new file i get 11
i'm sure something went wrong

@antpaw
Copy link
Owner

antpaw commented Oct 23, 2015

maybe it's not possible to have compat upto v1.2 anymore

@antpaw
Copy link
Owner

antpaw commented Oct 23, 2015

very strange, can you check if datepicker still works?

@majioa
Copy link
Contributor Author

majioa commented Oct 26, 2015

@antpaw updated

@antpaw
Copy link
Owner

antpaw commented Oct 26, 2015

thank you.
It would be much easier for me to merge if every change would go into different branch and it's own pull request. Stuff you are doing is not a hotfix, it's new features and hotfixes for this new features. New features need some descriptions, context and discussions.
Can you branch out of upstream/master and cherrypick the commits you want into each pull-request-branch? You can still have a master branch in your fork where all this is merged into one branch.

I don't understand why you want this csrf protection? This is protected admin area that can't be reached by a xss attack anyway.
I don't understand what this readonly feature does? If you have the attribute disabled set on the form elements, they will not send their values to the controller in the first place.

@majioa
Copy link
Contributor Author

majioa commented Oct 27, 2015

@antpaw This is protected admin area that can't be reached by a xss attack anyway.
How it is protected, by what?

@majioa
Copy link
Contributor Author

majioa commented Oct 27, 2015

If you have the attribute disabled set on the form elements, they will not send their values to the controller in the first place.

@antpaw yes, but, this does not protect it from changing by modified html. I'll play with disabled one

@antpaw
Copy link
Owner

antpaw commented Oct 27, 2015

I mean the login barrier, (you build an extra feature for it lately, something related to "device" gem) if someone can login, why would they try to do an xss attack after that. They can just clear the hole database or flood it within seconds.

If you want to protect attributes, do it in the model (this would be just one out of many other options https://github.com/rails/protected_attributes) Bhf is designed to have almost no business logic inside the controller because the bhf::controller can not be easily extended like a model or the view.

@majioa
Copy link
Contributor Author

majioa commented Oct 27, 2015

@antpaw the delete feature just require that authentication by validating the scrf-token. So in order to remove a record, rails just makes sure that you are admin...
by-the-way protected attributes is obsolete techinique....

@antpaw antpaw closed this Nov 1, 2015
@majioa
Copy link
Contributor Author

majioa commented Nov 2, 2015

Here I'll put fixups... dont close the branch

@antpaw antpaw reopened this Nov 2, 2015
@antpaw
Copy link
Owner

antpaw commented Nov 2, 2015

ok

@antpaw
Copy link
Owner

antpaw commented Nov 2, 2015

i don't see how this branch, can ever be merged, unless you do some heavy git history modification and use git push --force, it should be much simpler for you (and me) to just branch out from upstream/master and do the hotfixes

@majioa
Copy link
Contributor Author

majioa commented Nov 2, 2015

Yes, I'll use git force.

@antpaw antpaw force-pushed the master branch 3 times, most recently from 0a6fc2a to 3998e21 Compare December 27, 2019 17:21
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