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

Samba share page not displaying correct information for existing shares #1176

Closed
nicolaslt opened this Issue Feb 20, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@nicolaslt
Copy link
Contributor

nicolaslt commented Feb 20, 2016

Hi,

After creating a share, when going back to it via the 'edit' button, some -if not most- of the inputs do not display the correct data. Example: Read Only always stays on yes.

I was on my way to make a pull request for this but it seems to me this page needs more work. I'm happy to do that extra work if we agree it's needed.

There are many duplications in the .jst as follow:

{{#if sambaShareIdNotNull}}
    <input type="text" id="comment" class="form-control" name="comment" value="{{smbShareComment}}" title="Comment string to associate with the new share">
{{else}}
    <input type="text" id="comment" class="form-control" name="comment" value="Samba-Export" title="Comment string to associate with the new share">
{{/if}}

which IMHO should be:

<input type="text" id="comment" class="form-control" name="comment" value="{{smbShareComment}}" title="Comment string to associate with the new share" placeholder="Samba-Export">

And the javascript defines 5 handlebar helpers to render 3 yes/no radio buttons and 2 selects. Couple of bullets for this:

  • I guess the yes/no radios should be checkboxes
  • Those helpers defeat the purpose of having a templating engine. Why not use a handlebar partial instead ? Especially as the helpers duplicate the code by quite a bit. Each yes/no helper has 4 lines of html. (1 per permutation between checked and the option name and value)
  • This is where the original issue comes from; there's a few this and scope confusion in the building of the yes/no radios.

@schakrava schakrava added this to the Kilauea Iki milestone Feb 23, 2016

@schakrava schakrava added the bug label Feb 23, 2016

@schakrava schakrava self-assigned this Feb 23, 2016

schakrava added a commit to schakrava/rockstor-core that referenced this issue Feb 24, 2016

fix radio button presets when editing an export. rockstor#1176
Just a functional fix which is part 1 of 2. handlebar helpers and
templating must be improved significantly.

schakrava added a commit to schakrava/rockstor-core that referenced this issue Feb 24, 2016

@schakrava

This comment has been minimized.

Copy link
Member

schakrava commented Feb 24, 2016

@nicolaslt thanks for opening this issue and nicely articulating the shortcomings for current handlebar-code and suggesting improvements. Totally agree with you.

I've fixed up the functional bugs in the commits referred above. But @priyaganti is owning the templating improvements and so I am keeping this issue open for a part2 pull request from her.

Btw, thanks for becoming a code contributor. We welcome pull requests and would greatly appreciate your time and collaboration. Please don't be shy to get more involved.

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

nicolaslt commented Feb 25, 2016

Thanks @schakrava. I'll test this fix on my setup sometime next week I think.

About contributing: For now I'm only taking care of issues I encounter on my own setup. I might get more involved in a bit. Got other priorities right now. :-)

@schakrava

This comment has been minimized.

Copy link
Member

schakrava commented Feb 28, 2016

Totally understandable. Feel free to get involved as and when you are ready.

@nicolaslt

This comment has been minimized.

Copy link
Contributor Author

nicolaslt commented Mar 8, 2016

Finally found the time to test, it works fine. Thank @schakrava.

@schakrava schakrava assigned priyaganti and unassigned schakrava Apr 3, 2016

@schakrava schakrava closed this in e7427bb Apr 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.