Rework edituser page to allow user selection of a default dashboard #4551

Merged
merged 2 commits into from Oct 11, 2016

Projects

None yet

8 participants

@jonathon-k
Contributor

Pulls most edit user fields into functions to allow selective construction of the edituser page.
If can_update_users() returns 1 all applicable fields are displayed otherwise only the default dashboard select and twofactor auth sections display.

@laf
Member
laf commented Sep 23, 2016

I'm not sure I'm a fan of how this is done to be honest, building pages using functions seems a bit odd.

@librenms/reviewers

@paulgear
Member

I'm not sure how building pages with functions could be worse than doing it with include files or just single monolithic pages. It certainly gives the opportunity to make things more self-contained. If the change works, I'm +1 for splitting it up with functions.

@laf
Member
laf commented Sep 24, 2016

The include files way is the core of how it all works (rightly or wrongly) so changing that is difficult. If we ended up with 1,000 of functions to build all aspects of all our pages then that's just seems crazy to me and if so doing it on only one or a handful of pages is just even worse as it's then inconsistent across the entire code base.

@jonathon-k
Contributor
jonathon-k commented Sep 24, 2016 edited

I was hoping to make the if blocks less cluttered, but the same page change could be done with variables that are created and then assigned inside the ifs and combined at the end to create the final output string.

Are you okay with the logic that performs the actual updates to the database and such staying in a function?

@murrant
Contributor
murrant commented Sep 24, 2016

I would prefer to stick with the same style for consistency.

V2 is the place to make big changes and because we use blade templates there, things are much better.

@laf
Member
laf commented Sep 27, 2016

I'll be honest I'm still confused over this code, we're building a variable with all the html code to just print it out rather than just echoing as we go along.

@jonathon-k
Contributor

Yeah, so some of the fields could be interleaved without redundant checks
for being able to update the user. If the preference is for echo as you
go... I'll do that and do some re-ordering.

On Tue, Sep 27, 2016 at 12:35 PM, Neil Lathwood notifications@github.com
wrote:

I'll be honest I'm still confused over this code, we're building a
variable with all the html code to just print it out rather than just
echoing as we go along.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4551 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AUYaPxxxSvcm1C2i4XbGos5IgJmOxC5Gks5quWINgaJpZM4KEaj0
.

@laf
Member
laf commented Sep 27, 2016

Overall we just want a clean codebase. We don't have that now, a lot because of the original code from the fork but we should be trying to tidy things up rather than (and this is my opinion) make it harder to read. Others might disagree with me so it's worth hearing from others before you go off and do more work on this.

@laf
Member
laf commented Oct 3, 2016

tagging @librenms/reviewers for comment

@Rosiak
Contributor
Rosiak commented Oct 3, 2016

I really appreciate the effort put into this, but I am in the same boat as @murrant .

Jonathon Koyle Edit user echo as you go
6e89fbd
Jonathon Koyle Style fixes
ec84c3a
@scrutinizer-notifier

The inspection completed: No new issues

@f0o
Member
f0o commented Oct 5, 2016

trigger ci

@f0o
f0o approved these changes Oct 5, 2016 View changes
@laf laf added the WebUI label Oct 11, 2016
@laf laf merged commit 7d3cafd into librenms:master Oct 11, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@worton worton added a commit to worton/librenms that referenced this pull request Oct 11, 2016
@worton jonathon-k + worton refactor: edituser page to allow user selection of a default dashboard ( 00b0edb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment