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

Deprecate the default rooms beta for #admins and #announce rooms #18393

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@ const defaultProps = {
* Get the most recently accessed report for the user
*
* @param {Object} reports
* @param {Boolean} [ignoreDefaultRooms]
* @param {Boolean} [ignoreDomainRooms]
* @param {Object} policies
* @param {Boolean} isFirstTimeNewExpensifyUser
* @param {Boolean} openOnAdminRoom
* @returns {Object}
*/
const getInitialReportScreenParams = (reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => {
const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom);
const getInitialReportScreenParams = (reports, ignoreDomainRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom) => {
const last = ReportUtils.findLastAccessedReport(reports, ignoreDomainRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom);

// Fallback to empty if for some reason reportID cannot be derived - prevents the app from crashing
const reportID = lodashGet(last, 'reportID', '');
Expand Down
1 change: 0 additions & 1 deletion src/libs/OptionsListUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,6 @@ function getMemberInviteOptions(
return getOptions([], personalDetails, {
betas,
searchInputValue: searchValue.trim(),
excludeDefaultRooms: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed this because I found that this param wasn't being used anywhere in getOptions

includePersonalDetails: true,
excludeLogins,
sortPersonalDetailsByAlphaAsc: false,
Expand Down
17 changes: 9 additions & 8 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,13 +318,13 @@ function isConciergeChatReport(report) {

/**
* @param {Record<String, {lastReadTime, reportID}>|Array<{lastReadTime, reportID}>} reports
* @param {Boolean} [ignoreDefaultRooms]
* @param {Boolean} [ignoreDomainRooms]
* @param {Object} policies
* @param {Boolean} isFirstTimeNewExpensifyUser
* @param {Boolean} openOnAdminRoom
* @returns {Object}
*/
function findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom = false) {
function findLastAccessedReport(reports, ignoreDomainRooms, policies, isFirstTimeNewExpensifyUser, openOnAdminRoom = false) {
// If it's the user's first time using New Expensify, then they could either have:
// - just a Concierge report, if so we'll return that
// - their Concierge report, and a separate report that must have deeplinked them to the app before they created their account.
Expand All @@ -339,16 +339,17 @@ function findLastAccessedReport(reports, ignoreDefaultRooms, policies, isFirstTi
return _.find(sortedReports, report => !isConciergeChatReport(report));
}

if (ignoreDefaultRooms) {
// We allow public announce rooms to show as the last accessed report since we bypass the default rooms beta for them.
if (ignoreDomainRooms) {
// We allow public announce rooms, admins, and announce rooms through since we bypass the default rooms beta for them.
// Check where ReportUtils.findLastAccessedReport is called in MainDrawerNavigator.js for more context.
sortedReports = _.filter(sortedReports, report => !isDefaultRoom(report) || isPublicAnnounceRoom(report)
// Domain rooms are now the only type of default room that are on the defaultRooms beta.
sortedReports = _.filter(sortedReports, report => !isDomainRoom(report)
|| getPolicyType(report, policies) === CONST.POLICY.TYPE.FREE
Comment on lines -345 to 347
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

|| hasExpensifyGuidesEmails(lodashGet(report, ['participants'], [])));
}

let adminReport;
if (!ignoreDefaultRooms && openOnAdminRoom) {
if (openOnAdminRoom) {
adminReport = _.find(sortedReports, (report) => {
const chatType = getChatType(report);
return chatType === CONST.REPORT.CHAT_TYPE.POLICY_ADMINS;
Expand Down Expand Up @@ -1494,8 +1495,8 @@ function canSeeDefaultRoom(report, policies, betas) {
return true;
}

// Include any public announce rooms, since they could include people who should have access but we don't know to add to the beta
if (report.visibility === CONST.REPORT.VISIBILITY.PUBLIC_ANNOUNCE) {
// Include any admins and announce rooms, since only non partner-managed domain rooms are on the beta now.
if (isAdminRoom(report) || isAnnounceRoom(report)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All #admins, #announce rooms are free policy so not able to reach this condition as early returned here:

App/src/libs/ReportUtils.js

Lines 1495 to 1497 in 5f649d5

if (getPolicyType(report, policies) === CONST.POLICY.TYPE.FREE) {
return true;
}

I had to remove this manually to test this PR properly.

return true;
}

Expand Down
8 changes: 4 additions & 4 deletions tests/unit/SidebarFilterTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,11 @@ describe('Sidebar', () => {
[`${ONYXKEYS.COLLECTION.REPORT}${report3.reportID}`]: report3,
}))

// Then no reports are rendered in the LHN
// Then all non-domain rooms are rendered in the LHN
.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
const optionRows = screen.queryAllByAccessibilityHint(hintText);
expect(optionRows).toHaveLength(0);
expect(optionRows).toHaveLength(2);
})

// When the user is added to the default policy rooms beta and the sidebar re-renders
Expand Down Expand Up @@ -222,11 +222,11 @@ describe('Sidebar', () => {
// When the policy is a paid policy
.then(() => Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.policyID}`, {type: CONST.POLICY.TYPE.CORPORATE}))

// Then the report is not rendered in the LHN
// Then the report is still rendered in the LHN
.then(() => {
const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
const optionRows = screen.queryAllByAccessibilityHint(hintText);
expect(optionRows).toHaveLength(0);
expect(optionRows).toHaveLength(1);
});
});

Expand Down