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

(dev/core#3141) Membership should be listed chronologically by join d… #23056

Merged
merged 1 commit into from
May 31, 2022

Conversation

yashodha
Copy link
Contributor

@yashodha yashodha commented Mar 29, 2022

…ate, the most recent member since first

Overview

All memberships (active and inactive memberships) should be listed chronologically by join date, the most recent member since first.
This is followed on Membership dashboard but not Member tab or Member Userdashboard. The behavior should be consistent.

Before

dmaster_member_dashboard

After

fixed chronologically by join date on Userdashoard for active/inactive members
Screenshot from 2022-03-30 11-00-08

@civibot
Copy link

civibot bot commented Mar 29, 2022

(Standard links)

@civibot civibot bot added the master label Mar 29, 2022
@mattwire
Copy link
Contributor

@yashodha I agree with this change. How are the other items on the user dashboard (such as events, contributions) sorted. I expect they should also be sorted by newest first.

@yashodha
Copy link
Contributor Author

@mattwire There is a regression for contribution userdashboard (refer : https://lab.civicrm.org/dev/core/-/issues/1763)
I will be submitting a separate PR for that as well. It would be great to get this merged independently. Thanks!

@BettyDolfing
Copy link

Test this please

@yashodha
Copy link
Contributor Author

@eileenmcnaughton @mattwire Can you please merge? This is pretty straight forward. Thanks!

@jaapjansma
Copy link
Contributor

@BettyDolfing and I did a review of this PR and we discovered the the order on the membership tab on the contact record is still not in chronological order. On the dashboard it is.

Wrong: Member tab on contact card

Screenshot_20220411_155705

Correct: Memberlist on dashboard

Screenshot_20220411_155722

@yashodha can you have another look into the member tab on the contact record? Thanks!

@jaapjansma jaapjansma added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Apr 11, 2022
@yashodha
Copy link
Contributor Author

yashodha commented Apr 18, 2022

@jaapjansma there are 3 screens to check for this behavior :

Member Dashboard
correct behavior

Member Userdashboard
this PR fixes the behavior as per Member Dashboard

Member tab
wrong behavior still. This will be fixed in upcoming PR's!

For reference, check https://lab.civicrm.org/dev/core/-/issues/3141
This PR can be merged as this is in sync with the correct behavior of Member Dashboard

@eileenmcnaughton
Copy link
Contributor

Per @yashodha I'm ok with merging this - it is a super-simple patch that adds a sort-by.

The most important consistency is within the same page - which @yashodha has indicated she plans to address

However, I do wonder if we should sort be end_date or end_date,start_date rather than start_date?

@yashodha
Copy link
Contributor Author

@eileenmcnaughton Good point. I only did start date because the Member Dashboard was following this.

Let me know the consensus on this one and I will make changes accordingly . Whatever we decide will have to make changes to all 3 screens to keep it consistent.
Thanks!

@eileenmcnaughton
Copy link
Contributor

I've added a note to include this in the next dev-digest

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

@yashodha there is no feedback given on this issue since it has been mentioned in the dev-digest. As CiviCRM is open source it is basically scratch-your-own-itch so I believe you can decide which field the sorting should be on and make sure it is consistent on all three pages (eventually with separate PR but it could also be done in one PR). Could you do that? Thanks!

@eileenmcnaughton
Copy link
Contributor

Opps looks like that digest hasn't actually gone out - will aim to get it out this weekend

@yashodha
Copy link
Contributor Author

@eileenmcnaughton this is straight forward fix and definitely a step up from current state and in sync with what is already there on Member Dashboard. I think this could be merged and if there's a future consensus we can update all the screens.

@eileenmcnaughton
Copy link
Contributor

OK - I accept that - the dev digest item has gone out now so people have had a chance to comment

@eileenmcnaughton eileenmcnaughton merged commit d41a8c1 into civicrm:master May 31, 2022
@yashodha
Copy link
Contributor Author

yashodha commented Jun 1, 2022

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants