-
-
Notifications
You must be signed in to change notification settings - Fork 826
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
dev/core#105 Manage PCP URL Wrong for the notification email under wo… #12093
Conversation
@aniesshsethh can you squash the commits now you have beaten jenkins (git rebase -i origin/master should do it - although you will need to use -f switch to push) |
@kcristiano are you able to review? |
@eileenmcnaughton I'll take a look. |
Can you detail the steps to reproduce? As you mention the notification email I won't be able to fully test on wpmaster.demo.civicrm.org so I'll take a look on one of my test sites. My initial concern is if we change the url to the backend, the creator of the PCP page will have to have a user account and access to wp-admin. Once we can recreate the issue we may want to look if there is a permission that we can add to the anonymous user to solve this issue. |
…rdpress white space issue more white space issue another white space another white space
@eileenmcnaughton done I believe this will largely effect users which have a seperate front end template. Also, I believe this URL is only used in the admin notification and not in the emails to the creator of the campaign. Please correct me if I am wrong |
@aniesshsethh Thank you for the details. I do see the issue in the initial notification emails. You are correct, the link sent in the admin notification should be a backend link (wp-admin as opposed to the basepage). However, this brings up a related issue. The creator of the page cannot edit it as the only way to edit is via the backend. In fact, the email to the creator of the page tells them that they must login to their account to edit the page. The notification of the approval also says this: The issue is the user cannot login in unless the profile used to create the PCP includes an account creation link. Therefore we should update the docs to reflect this. |
Okay, I am new and I am not sure if I am the right person for the job of updating the docs I think the docs suffeciently mention this, I think we need to update the language in the default template itself. Maybe additional clearer workflow around in regards to self editing of the PCP at the software level itself. |
@eileenmcnaughton @aniesshsethh I have tested this patch and the admin notification works as expected. The patch does fix the bug and can be committed from my testing on WP. We should also test on Drupal and Joomla even though forcing backend should not have any negative affect there. As far as the docs go, It does make it clear that a user must have an account to edit the page, but I did not see where the docs would let a CiviCRM Admin know that they have to require account creation in the profile used to create a PCP page if they want their constituents to be able to edit the page. In addition, adding language in the default template could make this message clearer to the user creating the PCP As this reads now there is no mention that if you want to manage your page that you need to create an account. |
ping @KarinG @jackrabbithanna I believe both of you have used PCP before would you care to comment on this? |
looking in all the classes that inherit from Base.php, in CRM/Utils/System Neither Drupal6, Drupal, Drupal8, DrupalBase, or Backdrop use that $forceBackend parameter to the url() function, there will be no affect there. Joomla.php and Wordpress.php are the only ones that do. I wonder if this has been a problem in Joomla too? I don't have any Joomla sites to test against, but looking at the code its hard to see how it would cause a problem. In Soap.php https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/Soap.php#L61 Looks good to merge to me. |
I'm giving this 'merge-ready' - I'd like to hear Joomla! input (maybe @lcdservices ?) but I did ping on the channel & I'm not sure how else to get it. However, I do think logically we have a question 'should this be frontend or backend' and if the answer is 'backend' then it makes sense that applies for both/all CMS & it's arguable how far we should go to test - esp on obscure functionality on PCPs |
…rdpress
Overview
Manage PCP URL Wrong for the notification email under wordpress
Before
https://domain/civi/?page=CiviCRM&q=civicrm/admin/pcp&reset=1
After
https://domain/wp-admin/admin.php?page=CiviCRM&q=civicrm/admin/pcp&reset=1
Technical Details
Just setting backend to true