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

Issue #899: don't show PCPs where the events or contribution pages are disabled or past the end date #20845

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

herbdool
Copy link
Contributor

@herbdool herbdool commented Jul 14, 2021

Fixes https://lab.civicrm.org/dev/core/-/issues/899

Overview

This will only show PCP links on the user dashboard if the event or contribution page is active, or if either is before the end date, if set.

Before

See https://lab.civicrm.org/dev/core/-/issues/899. If an event or contribution page is disabled the PCP links still show. They also show for events and contribution pages that are past their end date, if set.

The link was also broken if it was a contribution page.

After

This will only show PCP links on the user dashboard if the event or contribution page is active, or if either is before the end date, if set.

It also fixes the link to the event or contribution.

Technical Details

Adds joins to contribution pages or events so it can add more conditions.

@civibot
Copy link

civibot bot commented Jul 14, 2021

(Standard links)

@civibot civibot bot added the master label Jul 14, 2021
@herbdool
Copy link
Contributor Author

I've updated it to allow for empty end dates.

I wasn't sure about checking end dates, though it seems necessary since it's not practical for users to disable events after they are done. And would be useful for contribution pages too.

@mattwire
Copy link
Contributor

@herbdool I gave this a quick test and you end up with You do not have any active Personal Campaign pages..

I think it would be better to still list the inactive pages, indicate that they are expired/inactive and on the PCP page itself disable the donate link / indicate that the campaign is no longer active (but still be able to share the historical page so you can eg. see how much was raised).

@herbdool
Copy link
Contributor Author

@mattwire doesn't seem to me that the dashboard really needs to keep all that info for disabled contrib/event pages. They would still be visible on the admin side.

Perhaps we could be more conservative and still show them regardless of end date (take that part out of my PR) and only exclude them where is_active of pages is zero. That way at least they won't see fatal errors when they try to click on them.

And then a new issue could be for putting an "expired/inactive" indicator on the PCP admin page.

@mattwire
Copy link
Contributor

@herbdool My opinion was that as the dashboard is for the end user they would like to see a history of all their old pcp pages (eg. if they do a campaign every year). I am happy to be persuaded otherwise but it seems a simple fix to remove the links instead of the whole line?

@herbdool
Copy link
Contributor Author

herbdool commented Jul 23, 2021

@mattwire I've updated the PR to show any existing PCPs for the user (even for inactive pages), but for the "Create a PCP..." only show contribution or event pages which are active and end in the future. Is that better?

Screenshot from 2021-07-23 12-25-59

I also fixed the query so it actually includes an end_date for the template. Before, all rows showed "Ongoing" regardless of the actual end date. Odd, that there were a few places in the template where the variables output weren't being passed at all. PCPs not getting much love.

Also, I removed a float from the "Create a PCP..." section. Looked ugly and broken even though it was done on purpose years ago I believe.

After:

Screenshot from 2021-07-23 14-02-07

@mattwire
Copy link
Contributor

@herbdool That sounds fine to me! Looks like there is a test failure caused by this change? That might be expected but can you check and fix the test if necessary?
Once done please squash the commits and I will test again.

@herbdool
Copy link
Contributor Author

I should get around to running Civi tests locally. This is taking a long time for small tweaks.

@herbdool
Copy link
Contributor Author

@mattwire finally. A few tries there.

I also finally got the buildkit set up locally; that took awhile.

@herbdool
Copy link
Contributor Author

@mattwire by the way, what's the best way to squash commits once the PR has already been pushed? I'm used to Backdrop PRs where the person merging does the squash.

@eileenmcnaughton
Copy link
Contributor

@herbdool just looking at your large question - you can use git rebase -i - although in this case you have a few commits so I would

git pull --rebase origin master
git reset origin/master
git add -p
git commit -m "dev/core#899 my sensible commit message"

One more thing - there is a preferred syntax for referring to the gitlab issue - you can copy it from within gitlab if you scroll down & look for the little clipboard symbol next to it

image

@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

jaapjansma commented Jan 24, 2022

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR and in the issue on Gitlab
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We followed the replication steps from https://lab.civicrm.org/dev/core/-/issues/899 and then disabled the contribution page, set end date and checked the user dashboard. When the contribution page is disabled or when the contribution has a past end date it is not possible to create a new personal campaign page. When the contribution page is active we could create a personal campaign page from the user dashboard.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change sufficiently improves test coverage
    • Test results (r-test)
      • PASS: The test results are all-clear.

@colemanw or @eileenmcnaughton or @totten can one of you merge this PR?

@colemanw colemanw merged commit 1e5f06c into civicrm:master Jan 24, 2022
@colemanw
Copy link
Member

Thanks @herbdool @mattwire @BettyDolfing @jaapjansma and @eileenmcnaughton - good group effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants