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.

Sign up for GitHub

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

[ENG-6669] Institutional Access project Request Modal #2433

Merged
merged 24 commits into from
Jan 10, 2025
Merged

[ENG-6669] Institutional Access project Request Modal #2433

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
51fd954
Merge branch 'hotfix/24.09.1'
mfraezz Nov 21, 2024
3a29a83
Merge branch 'release/24.10.0'
mfraezz Dec 11, 2024
5a7fe94
add Project request modal with user messaging tab to the institution…
Dec 18, 2024
13f0699
Merge branch 'feature/institutional-access' of https://github.com/Cen…
Dec 18, 2024
0039fc6
Feature/share download integration (#2457)
futa-ikeda Jan 6, 2025
660cee2
Bump version no. Add CHANGELOG.
adlius Jan 6, 2025
2fe05c7
Merge branch 'release/25.01.0'
adlius Jan 6, 2025
5300c97
Merge tag '25.01.0' into develop
adlius Jan 6, 2025
002ccc8
Feature/share download integration (#2457)
futa-ikeda Jan 6, 2025
bf8ab2c
Bump version no. Add CHANGELOG.
adlius Jan 6, 2025
1abd4e4
Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…
Jan 6, 2025
d54a71d
Fix funder column propertyPathKey
futa-ikeda Jan 6, 2025
dae4684
Update CHANGELOG, bump version
mfraezz Jan 6, 2025
6fb0bee
Merge branch 'hotfix/25.01.1' into develop
mfraezz Jan 6, 2025
9b21ce4
Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…
Jan 6, 2025
524eef0
integrations fixes and refactors
Jan 7, 2025
4057df3
add project request and messaging modal to institution dashboard
Jan 7, 2025
19fb416
Merge branch 'institutional-access-project-request-modal' of https://…
Jan 9, 2025
4b74341
Code review cleanup and typos
Jan 9, 2025
983ac69
clean-up user message pane and make tasks
Jan 9, 2025
e7f569d
use variables for colors
Jan 9, 2025
715bee8
clean-up exception handling and include notes
Jan 9, 2025
a92edbf
add translation to default value for unknown contributor
Jan 9, 2025
9a3e056
clean-up translation keys
Jan 10, 2025
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
14 changes: 14 additions & 0 deletions app/adapters/node-request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// app/adapters/user-message.js
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
import { inject as service } from '@ember/service';
import config from 'ember-osf-web/config/environment';
const { OSF: { apiUrl } } = config;
import OsfAdapter from './osf-adapter';

export default class NodeRequestAdapter extends OsfAdapter {
@service session;

urlForCreateRecord(modelName, snapshot) {
const nodeId = snapshot.record.target;
return `${apiUrl}/v2/nodes/${nodeId}/requests/`;
}
}
2 changes: 1 addition & 1 deletion app/adapters/user-message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import OsfAdapter from './osf-adapter';
export default class UserMessageAdapter extends OsfAdapter {
@service session;
urlForCreateRecord(modelName, snapshot) {
const userId = snapshot.record.user;
const userId = snapshot.record.messageRecipient;
return `${apiUrl}/v2/users/${userId}/messages/`;
}
}
6 changes: 3 additions & 3 deletions app/institutions/dashboard/-components/institutional-users-list/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default class InstitutionalUsersList extends Component<InstitutionalUsers
@tracked filteredUsers = [];
@tracked messageModalShown = false;
@tracked messageText = '';
@tracked bcc_sender = false;
@tracked bccSender = false;
@tracked replyTo = false;
@tracked selectedUserId = null;
@service toast!: Toast;
Expand Down Expand Up @@ -291,7 +291,7 @@ export default class InstitutionalUsersList extends Component<InstitutionalUsers
@action
resetModalFields() {
this.messageText = '';
this.bcc_sender = false;
this.bccSender = false;
this.replyTo = false;
this.selectedUserId = null;
}
Expand All @@ -308,7 +308,7 @@ export default class InstitutionalUsersList extends Component<InstitutionalUsers
const userMessage = this.store.createRecord('user-message', {
messageText: this.messageText.trim(),
messageType: MessageTypeChoices.InstitutionalRequest,
bcc_sender: this.bcc_sender,
bccSender: this.bccSender,
replyTo: this.replyTo,
institution: this.args.institution,
user: this.selectedUserId,
Expand Down
2 changes: 1 addition & 1 deletion app/institutions/dashboard/-components/institutional-users-list/template.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@
</div>
<div local-class='checkbox-container'>
<label local-class='checkbox-item'>
<Input @type='checkbox' @checked={{this.bcc_sender}} />
<Input @type='checkbox' @checked={{this.bccSender}} />
{{t 'institutions.dashboard.send_message_modal.cc_label'}}
</label>
<label local-class='checkbox-item'>
Expand Down
140 changes: 140 additions & 0 deletions app/institutions/dashboard/-components/object-list/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ import InstitutionModel from 'ember-osf-web/models/institution';
import { SuggestedFilterOperators } from 'ember-osf-web/models/related-property-path';
import SearchResultModel from 'ember-osf-web/models/search-result';
import { Filter } from 'osf-components/components/search-page/component';
import { waitFor } from '@ember/test-waiters';
import { task } from 'ember-concurrency';
import Toast from 'ember-toastr/services/toast';
import Intl from 'ember-intl/services/intl';
import Store from '@ember-data/store';
import CurrentUser from 'ember-osf-web/services/current-user';
import {MessageTypeChoices} from 'ember-osf-web/models/user-message';
import {RequestTypeChoices} from 'ember-osf-web/models/node-request';

import config from 'ember-osf-web/config/environment';

const shareDownloadFlag = config.featureFlagNames.shareDownload;
Expand Down Expand Up @@ -49,6 +58,20 @@ export default class InstitutionalObjectList extends Component<InstitutionalObje
@tracked sortParam?: string;
@tracked visibleColumns = this.args.columns.map(column => column.name);
@tracked dirtyVisibleColumns = [...this.visibleColumns]; // track changes to visible columns before they are saved
@tracked selectedPermissions = 'write';
@tracked projectRequestModalShown = false;
@tracked activeTab = 'request-access'; // Default tab
@tracked messageText = '';
@tracked bccSender = false;
@tracked replyTo = false;
@tracked selectedUserId = '';
@tracked selectedNodeId = '';
@tracked showSendMessagePrompt = false;
@service toast!: Toast;
@service intl!: Intl;
@service store!: Store;
@service currentUser!: CurrentUser;


get queryOptions() {
const options = {
Expand Down Expand Up @@ -151,4 +174,121 @@ export default class InstitutionalObjectList extends Component<InstitutionalObje
updatePage(newPage: string) {
this.page = newPage;
}

@action
openProjectRequestModal(contributor: any) {
this.selectedUserId = contributor.userId;
this.selectedNodeId = contributor.nodeId;
this.projectRequestModalShown = true;
}

@action
handleBackToSendMessage() {
this.activeTab = 'send-message';
this.showSendMessagePrompt = false;
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we have a setTimeout here? I'd like to avoid any unneeded timeouts if possible

Copy link
Collaborator Author

@Johnetordoff Johnetordoff Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I'll leave a comment here, I clarified this but only later on line 243, and not very well. The reason for the timeout is that ember bans having two dialogue modals open at the same time and the modal closing animation happens too slowly, so just

modal1.close()
modal2.open()

opens modal2 before the modal1 close animation is complete. Causing the framework to produce an error because there are technically two open modals. Product wanted a modal on this, because the toast message is not enough.

this.projectRequestModalShown = true; // Reopen the main modal
}, 200);

}

@action
closeSendMessagePrompt() {
this.showSendMessagePrompt = false; // Hide confirmation modal without reopening
}

@action
toggleProjectRequestModal() {
this.projectRequestModalShown = !this.projectRequestModalShown;
}

@action
updateselectedPermissions(permission: string) {
this.selectedPermissions = permission;
}

@action
setActiveTab(tabName: string) {
this.activeTab = tabName;
}


@action
resetFields() {
this.selectedPermissions = 'write';
this.bccSender = false;
this.replyTo = false;
}

@task
@waitFor
async handleSend() {
try {
if (this.activeTab === 'send-message') {
await this._sendUserMessage();
} else if (this.activeTab === 'request-access') {
await this._sendNodeRequest();
}

this.toast.success(
this.intl.t('institutions.dashboard.object-list.request-project-message-modal.message_sent_success'),
);
this.resetFields();
} catch (error) {
const errorDetail = error?.errors?.[0]?.detail.user || error?.errors?.[0]?.detail || '';
const errorCode = parseInt(error?.errors?.[0]?.status, 10);

if (errorCode === 400 && errorDetail.includes('does not have Access Requests enabled')) {
setTimeout(() => {
this.showSendMessagePrompt = true; // Timeout to allow the modal to exit
}, 200);
Comment on lines +245 to +247
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to a comment above, I would avoid using setTimeout to set page states if possible. In this case, I think you can just add this bit of logic in to the finally block like:

} finally {
  this.projectRequestModalShown = false; // Close the main modal
  if (relevantCondition) {
     this.showSendMessagePrompt = true;
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned in the comment above this actual will temporarily cause where both modals are open simultaneously.

} else if ([409, 400, 403].includes(errorCode)) {
// Handle specific errors
this.toast.error(errorDetail);
} else if (errorDetail.includes('Request was throttled')) {
this.toast.error(errorDetail);
} else if (errorDetail === 'You cannot request access to a node you contribute to.') {
this.toast.error(errorDetail);
} else {
this.toast.error(
this.intl.t('institutions.dashboard.object-list.request-project-message-modal.message_sent_failed'),
);
}
} finally {
this.projectRequestModalShown = false; // Close the main modal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like there's some redundant conditions here in this catch block as most of these seem to just result in the same toast message. Some of the conditions may be a bit fragile as well if they are doing a String.includes
and if the language returned from the API changes, these could break as well. I would recommend using getApiErrorMessage(error) and this should just find the appropriate error message from the API. Then at the end, you could just call this.toast.error(getApiErrorMessage(error), someOptionalToastTitle)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes this is because there isn't standardization of error messages, so some http response codes are in the payload and some are simply not there. the 429s and some 409s are not standard, however looking at this now and I think I can improve, so that it reads the response header instead of the payload.

}

async _sendUserMessage() {
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
const userMessage = this.store.createRecord('user-message', {
messageText: this.messageText.trim(),
messageType: MessageTypeChoices.InstitutionalRequest,
bccSender: this.bccSender,
replyTo: this.replyTo,
institution: this.args.institution,
messageRecipient: this.selectedUserOsfGuid,
});
await userMessage.save();
}

async _sendNodeRequest() {
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
const nodeRequest = this.store.createRecord('node-request', {
comment: this.messageText.trim(),
requestType: RequestTypeChoices.InstitutionalRequest,
requestedPermissions: this.selectedPermissions,
bccSender: this.bccSender,
replyTo: this.replyTo,
institution: this.args.institution,
messageRecipient: this.selectedUserOsfGuid,
target: this.selectedNodeId,
});
await nodeRequest.save();
}

get selectedUserOsfGuid() {
const url = new URL(this.selectedUserId);
const pathSegments = url.pathname.split('/').filter(Boolean);
return pathSegments[pathSegments.length - 1] || ''; // Last non-empty segment
}

}
11 changes: 10 additions & 1 deletion app/institutions/dashboard/-components/object-list/contributors-field/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Component from '@glimmer/component';
import { inject as service } from '@ember/service';
import Intl from 'ember-intl/services/intl';
import { action } from '@ember/object';

import InstitutionModel from 'ember-osf-web/models/institution';
import SearchResultModel from 'ember-osf-web/models/search-result';
Expand All @@ -10,6 +11,7 @@ import { getOsfmapObjects, getSingleOsfmapValue, hasOsfmapValue } from 'ember-os
interface ContributorsFieldArgs {
searchResult: SearchResultModel;
institution: InstitutionModel;
projectRequestModal: (contributor: any) => void;
}

const roleIriToTranslationKey: Record<AttributionRoleIris, string> = {
Expand Down Expand Up @@ -48,12 +50,19 @@ export default class InstitutionalObjectListContributorsField extends Component<
const contributor = getContributorById(contributors, getSingleOsfmapValue(attribution, ['agent']));
const roleIri: AttributionRoleIris = getSingleOsfmapValue(attribution, ['hadRole']);
return {
name: getSingleOsfmapValue(contributor,['name']),
name: getSingleOsfmapValue(contributor, ['name']) || 'Unknown Contributor',
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
userId: contributor['@id'],
nodeId: searchResult.indexCard.get('osfGuid'),
url: getSingleOsfmapValue(contributor, ['identifier']),
permissionLevel: this.intl.t(roleIriToTranslationKey[roleIri]),
};
});
}

@action
handleOpenModal(contributor: any) {
this.args.projectRequestModal(contributor);
}
}

function hasInstitutionAffiliation(contributors: any[], attribution: any, institutionIris: string[]) {
Expand Down
14 changes: 14 additions & 0 deletions app/institutions/dashboard/-components/object-list/contributors-field/styles.scss
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically discourage the use of !important in css files, as these could lead to weird one-off styles. If you can achieve the same styles without the !important (which at first glance, I don't see anything that needs it), that would be appreciated. If it is needed, I usually leave a little comment to indicate what it is overriding or why it's needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I keep having issues with Ember "Uppercase" component's css, especially <Button> button's need !important to override the no border rule, because (I believe) buttons had an accessibility requirement to have boarders at some point. I'll try to fix it, but I'll leave a long comment if I can't fix it.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

.icon-message {
opacity: 0;
color: $color-text-blue-dark;
background-color: inherit !important;
border: 0 !important;
box-shadow: 0 !important;
}

.icon-message:hover {
opacity: 1;
background-color: inherit !important;
}

7 changes: 7 additions & 0 deletions app/institutions/dashboard/-components/object-list/contributors-field/template.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@
</OsfLink>
{{t 'institutions.dashboard.object-list.table-items.permission-level' permissionLevel=contributor.permissionLevel}}
</div>
<Button
local-class='icon-message'
aria-label={{t 'institutions.dashboard.request_project_message_modal.open_aira_label'}}
Johnetordoff marked this conversation as resolved.
Show resolved Hide resolved
{{on 'click' (fn this.handleOpenModal contributor)}}
>
<FaIcon @icon='comment' />
</Button>
{{else}}
<div>
{{t 'institutions.dashboard.object-list.table-items.no-contributors'}}
Expand Down
68 changes: 68 additions & 0 deletions app/institutions/dashboard/-components/object-list/styles.scss
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, on this CSS file, there's a couple "mystery colors" floating around. We typically try to use color variables like $color-border-gray instead of say #ddd. You might find colors that match in app/styles/_variables.scss, so I would encourage the use of the color variables in there

Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,74 @@
padding-left: 10px;
}

.message-textarea {
min-width: 450px;
min-height: 280px;
}

.message-label {
display: block;
}

.checkbox-container {
display: flex;
flex-direction: column;
gap: 10px;
padding: 10px;
}

.checkbox-item {
display: flex;
align-items: center;
gap: 8px;
font-size: 14px;
}

.radiobox-container {
gap: 20px;
}

.radiobox-item {
display: flex;
align-items: center;
gap: 8px;
font-size: 14px;
}

.tab-container {
display: flex;
border-bottom: 1px solid #ddd;
margin-bottom: 1rem;
}

.tab-button {
flex: 1;
padding: 10px;
text-align: center;
cursor: pointer;
background: #f9f9f9;
border: 0;
border-bottom: 2px solid transparent;

&.active {
border-bottom: 2px solid #007bff;
font-weight: bold;
}

&:hover {
background: #f1f1f1;
}
}

.tab-pane {
padding: 1rem;
background: #fff;
}

.request-modal {
max-width: min-content;
}

.download-dropdown-content {
display: flex;
flex-direction: column;
Expand Down
Loading
Loading