Skip to content

PDJB-1276: Org landlords on property details landlords tab - #1667

Open
samyou-softwire wants to merge 8 commits into
feat/PDJB-1275-fix-landlord-permission-checksfrom
feat/PDJB-1276-org-landlords-on-property-details-landlords-tab
Open

PDJB-1276: Org landlords on property details landlords tab#1667
samyou-softwire wants to merge 8 commits into
feat/PDJB-1275-fix-landlord-permission-checksfrom
feat/PDJB-1276-org-landlords-on-property-details-landlords-tab

Conversation

@samyou-softwire

@samyou-softwire samyou-softwire commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Ticket number

PDJB-1276

Goal of change

make it so org landlords show up on the property details list of landlords tab

screenshots my view landlords tab viewing as me

someone elses view
landlords tab viewing as someone else

Description of main change(s)

Anything you'd like to highlight to the reviewer?

Checklist

Delete any that are not applicable, and add explanation below for any that are applicable but haven't been done

  • Screenshots of any UI changes have been added
  • Controller tests for any new endpoints, including testing the relevant permissions
  • Single page integration tests have been added for any unhappy-flow UI features, e.g. validation errors
  • Test suite has been run in full locally and is passing
  • Branch has been rebased onto main and run locally, with everything working as expected (both for your new feature
    and any related functionality)
  • TODO comments referencing this JIRA ticket have been searched for and removed - if a future PR will address them,
    mention that here
  • QA instructions have been added to the ticket (particularly if this is the last PR required to complete the ticket)
  • This feature is not behind a feature flag. I've checked that this is appropriate and we're happy with this code becoming live as soon as we release

@samyou-softwire samyou-softwire self-assigned this Jul 30, 2026
@samyou-softwire
samyou-softwire changed the base branch from main to feat/PDJB-1275-fix-landlord-permission-checks July 30, 2026 17:26
@samyou-softwire
samyou-softwire force-pushed the feat/PDJB-1276-org-landlords-on-property-details-landlords-tab branch from bd8b30f to 00648ce Compare July 30, 2026 18:37
@samyou-softwire
samyou-softwire marked this pull request as ready for review July 31, 2026 09:22

@TomHanmer29 TomHanmer29 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a couple small thoughts :)

JOIN property_ownership po ON po.id = 1600 + i
ON CONFLICT DO NOTHING;

-- PDJB-1276: Add org landlord (11) as joint landlord on property 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Do we need to prefix this with the ticket number? Is this a convention we should adopt more widely? I think in general these files are becoming pretty big and hard to follow, but we're talking about them in catchup soon anyway

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think we should audit this file a bit; I wonder how much of this data was just added for a QA


assertEquals(2, detailsPage.landlordSummaryCards.size)
val currentUserCard = detailsPage.landlordSummaryCards[0]
assertEquals("Alexander Smith (you)", currentUserCard.title.getText())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add one of these to assert the (your organisation) text is working or is that a lot of setup for this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this; not big right now but I can imagine tests down the road will need to test views like this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay; the @WithOrgLandlordUser can be applied to an integration test class and will make you be logged in as an org landlord

@samyou-softwire

Copy link
Copy Markdown
Contributor Author

re requesting review on how the org landlord testing infrastructure looks


import org.springframework.test.context.ActiveProfiles

@ActiveProfiles(profiles = ["local", "local-no-auth", "local-org-landlord"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a fine way of doing it, it kinda sets the precedent that individual landlords are "normal" (as we have no annotation for them) and org landlords are "special" but I think that's true. The only similar thing to this is the @withMockUser annotation for landlords, LC and SO but I think these are separate enough concepts.

@samyou-softwire

Copy link
Copy Markdown
Contributor Author

waiting on base branch #1654 to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants