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

Display notifications correctly #287

Merged
merged 1 commit into from
May 20, 2019

Conversation

mathiashoeld
Copy link
Contributor

@mathiashoeld mathiashoeld commented May 16, 2019

Changes

Notification page

The notification page now works as expected. The user can click on the next page and see the next set of notifications.
Additionally, the magnifier button is now enabled when the user has permissions to see the project/subproject and disabled otherwise.

E2E-Test

The e2e tests for notifications have been updated and are working now.

Projected Budgets

The assignee of a project/subproject now gets notified if a projected budget was updated or deleted.

Fly-in notifications

The magnifier button in fly-in notifications now correctly redirects the user to the given project/subproject instead of throwing an error.

Notifications

When a user is assigned to a workflowitem, a notification is sent.

Issues closed

Closes #283
Closes #285
Closes #261
Closes #284
Closes #272

Copy link
Collaborator

@gonzochic gonzochic left a comment

Choose a reason for hiding this comment

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

I am missing the Unit-Tests which are testing the creation of the events for the adapted classes

additionalData: {},
};
const baseRepository = {
getSubprojects: async _projectId => [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this async?

},
};

describe("Update subproject projected budget: permissions", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: Update should be Delete (same for the whole file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in the file

root,
projectId,
subprojectId,
"Othertestcorp",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The id's don't match with the existing project budget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,274 @@
import { assert } from "chai";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add the following tests:

  • Delete existing PB and check if it is indeed deleted
  • Delete non-exsiting PB and check that no error occurs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done
also added unit test for updating PBs (project and subproject)

@mathiashoeld mathiashoeld force-pushed the 272-display-notifications-correctly branch from 9974e35 to 94c829a Compare May 20, 2019 07:51
gonzochic
gonzochic previously approved these changes May 20, 2019
Summary:
- Fix navigation on notification page
- Enable and fix e2e-test for notifications
- api: Notify assignee when projected budgets are updated or deleted
- api: Add unit tests for projected budgets
- api: Send notification to new workflowitem assignee and add unit tests
- api: adapted test script for new file structure
- ui: Fix enabling of redirect button on notification page
- ui: Fix redirect button in fly-in notifications
- ui: Fix redirect button in fly-in notifications
- ui: Fix language issue

Fix navigation on notification page:
- The navigation was disabled on the notification page - this was fixed
by setting the notification count correctly in the reducer
- The last page of the notifications overfetched the notifications -
this was fixed by calculating the limit for each page to display only
the remaining items.
- The notification message was changed for the case where the user is
not allowed to see the resource - "cannot fond project" was removed

e2e-tests: Fix test for notifications:
- Added command for closing a project
- Activated and fixed the test for notifications
- Added class names to read all button and notifications in the UI

Notify assignee when projected budgets are updated or deleted:
Since the assigned user gets notified when the project/subproject is updated,
he/she also should get notified when the projected budget is updated or
deleted.

- API/Domain layer: Added the creation of notification events for
project_projected_budget_update, project_projected_budget_delete ,subproject_projected_budget_update,subproject_projected_budget_delete
- API/Service layer: Added the function to resolve users and groups to
the repository that is called by the domain layer
- Frontend: Added the business event to the language file, so the
message can be displayed

Closes #283

ui: Fix enabling of redirect button on notification page
The button is now enabled when the user has the permissions to see the
project/subproject and vice versa.

Closes #284

ui: Fix redirect button in fly-in notifications
When the user gets a fly-in notification, the magnifier button now
redirects to the project/subproject mentioned in the notification.

ui: Fix language issue:
The entries for project_projected_budget_updated, project_projected_budget_deleted, subproject_projected_budget_updated, subproject_projected_budget_deleted are now located in the correct section of the german language file.

Minor Fixes:
- UI: Use 'data-test' labels to identify elements for testing in
- NotificationListItems
- E2E-Test: Use the 'data-test' label to query elements in the DOM
- API: Use consistent implementation for creation of notification events for projected budget updates/deletions

api: Add unit tests for projected budgets:
Add unit tests for project_projected_budget_update, project_projected_budget_delete,
subproject_projected_budget_update, subproject_projected_budget_delete to test sending of notifications.

api: adapted test script for new file structure
With the command 'npm run test:here name_of_test_case_file' you can now
test specific unit test files.

api: Send notification to new workflowitem assignee
The notification was sent to the old assignee instead of the new
assignee. Now the new assignee is notified if a workflow item is
assigned to him/her.

api: Send correct notifications after assigining workflow item
- The recipient for notifications after assigning workflowitems is now
the new assignee of the workflowitem as it should be
- Added unit test to test this behaviour
- Fixed typos in other unit tests and added one test case each

Add changelog entry
@mathiashoeld mathiashoeld merged commit 5fbea36 into master May 20, 2019
@mathiashoeld mathiashoeld deleted the 272-display-notifications-correctly branch May 20, 2019 15:33
@mathiashoeld mathiashoeld added this to the TruBudget 1.0.1 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment