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

Add UI to enable Admin Bar while Logged-In User caching is enabled; #245

Closed
wants to merge 9 commits into from

Conversation

renzms
Copy link
Contributor

@renzms renzms commented Apr 18, 2016

@raamdev
Copy link
Contributor

raamdev commented Apr 21, 2016

@renzms Please only label Pull Requests as ready for review, not the associated GitHub issue (as you did with wpsharks/comet-cache#690). Also, when working on a PR, please post screenshots and questions inside the PR, not inside the GitHub issue.


Regarding the PR: It looks like you're using when_logged_in as the option, however that's the option for Logged-In User Caching. You'll need to create a new option, e.g., admin_bar_when_logged_in, and use that. You'll also need to add the new option key to the default options list (with a default of 1 for Enabled, and the Pro-only options list (Logged-In User Caching is a Pro-only feature).

@raamdev
Copy link
Contributor

raamdev commented Apr 21, 2016

@renzms I forgot to say that the wording in the new options section looks awesome! Very nice work putting that together. :-) I only see a few minor tweaks necessary. I'll explain those once we get this PR in a working state.

@renzms
Copy link
Contributor Author

renzms commented Apr 22, 2016

@raamdev

Thanks for the review! Noted everything and moved the content here:

screen shot 2016-04-18 at 11 16 46 pm

Work in progress, I tested it but I think I need help with the globals and options I used. Mixed up the logic somewhere and its overriding the Logged-In User Caching option.

Added it as new section in Logged-In User Caching section, all text is placeholder of course. Any suggestions regarding the text?

Updated description and document to fit current version number for added feature.
@renzms
Copy link
Contributor Author

renzms commented Apr 23, 2016

@raamdev

Hi Raam, updated according to your notes. Please review, still a work in progress. Thanks!

@raamdev
Copy link
Contributor

raamdev commented Apr 24, 2016

@renzms Thanks! Here are a few more notes:

  • ADMIN_BAR_WHEN_LOGGED_IN: please see the other constants for the correct format for this.
  • Please rename admin_bar_when_logged_in to when_logged_in_admin_bar; after reviewing the other plugin options, I noticed the format we're using is to start the the option name with the area of the plugin it belongs to (e.g., cache_clear_eval_code, cache_clear_urls), so when_logged_in_admin_bar makes more sense.

Also, you have a build failure that you'll need to fix.

@renzms
Copy link
Contributor Author

renzms commented Apr 29, 2016

@raamdev

Hi Raam, updated according to your notes.

I still have one build failure to fix, but I'm not clear on the error:

Error in /bootstrap-ci/src/travis:58
`docker run --detach --hostname=ci.vm --add-host=ci.vm:127.0.0.1 --add-host=sub.ci.vm:127.0.0.1 --add-host=sub1.ci.vm:127.0.0.1 --add-host=sub2.ci.vm:127.0.0.1 --add-host=cdn.ci.vm:127.0.0.1 --add-host=cdn1.ci.vm:127.0.0.1 --add-host=cdn2.ci.vm:127.0.0.1 --name="${docker_container_name}" --volume=/bootstrap-ci:/bootstrap-ci --volume="${CI_CFG_PROJECT_DIR}":"${CI_CFG_PROJECT_DIR}" "${docker_image_name_tag}"` exited with status `0`.
Stack Trace:
 1: ./.travis.bash:14 source(...)
Exiting w/ status `1`.

@raamdev
Copy link
Contributor

raamdev commented May 2, 2016

@renzms Have you git pull and merged 000000-dev and git submodule update?

@renzms
Copy link
Contributor Author

renzms commented May 3, 2016

Have you git pull and merged 000000-dev and git submodule update?

@raamdev Yup! As per your instructions previously, and I do that now often to make sure there are no merge conflicts.

@raamdev
Copy link
Contributor

raamdev commented May 6, 2016

@renzms Well somehow your local branch is still out of date, along with the remote copy of the branch here on GitHub.

When I select your feature/690 branch on GitHub, look what revision the Phings module is at (1b75e4e) and notice how GitHub says that the branch is "4 commits ahead and 8 commits behind 000000-dev":

2016-05-06_17-58-31
2016-05-06_17-59-38

Your branch is using an older version of Phings (v160411) and 000000-dev is using the latest version of Phings (v160418).

You need to get your branch synced up by pulling down the latest changes from the 000000-dev branch on GitHub, merging those into feature/690, updating the submodule with git submodule update, and then pushing those changes to the branch back up to GitHub.

@renzms
Copy link
Contributor Author

renzms commented May 10, 2016

@raamdev

Conflicts fixed and check now successful. Ready for review. Thanks!

@raamdev
Copy link
Contributor

raamdev commented May 10, 2016

@renzms Looks like something is wrong with the options panel (also, the GET Requests option panel is now missing entirely):

2016-05-10_13-27-18

@raamdev
Copy link
Contributor

raamdev commented May 12, 2016

@renzms This bug also needs to be fixed as part of this PR: wpsharks/comet-cache#690 (comment)

@renzms
Copy link
Contributor Author

renzms commented May 13, 2016

@raamdev

Pushed a commit to address:

Looks like something is wrong with the options panel (also, the GET Requests option panel is now missing entirely):

and

This bug also needs to be fixed as part of this PR: wpsharks/comet-cache#690 (comment)

screen shot 2016-05-13 at 5 55 52 pm


I noticed during testing though that Logged - In User Caching options were stuck at Yes, but DON'T maintain a separate cache for each user (I know what... when trying to change the option. So I restored options and cleared the cache, then uninstalled my phing build of this PR. Installed Comet Cache™ Pro v160417 to see if it would fix it, but the option is still stuck at Yes... so my admin bar is disabled while Comet Cache is enabled. (This is a clean install, no other plugins enabled and default theme applied)

screen shot 2016-05-13 at 6 08 41 pm

@raamdev
Copy link
Contributor

raamdev commented May 13, 2016

@renzms writes...

I noticed during testing though that Logged - In User Caching options were stuck at Yes, but DON'T maintain a separate cache for each user (I know what... when trying to change the option. So I restored options and cleared the cache, then uninstalled my phing build of this PR. Installed Comet Cache™ Pro v160417 to see if it would fix it, but the option is still stuck at Yes...

I just tested Comet Cache Pro v160417 in a clean install and I have no problem changing the Logged-In User Caching option and saving the changes.

@raamdev
Copy link
Contributor

raamdev commented May 13, 2016

@renzms FYI, I pushed a few commits to this branch after running a few tests:

2016-05-13_18-18-06

I'm going to merge this in now, as it looks ready for release.

@raamdev raamdev closed this May 13, 2016
@raamdev raamdev deleted the feature/690 branch May 13, 2016 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants