Add commenting for fuel suppliers #414

plasticviking · 2018-06-29T18:28:56Z

This pull request introduces the commenting feature for fuel suppliers.

Changelog highlights:

Added natural key lookups for User, Role, Permission, Credit Trade Status, and Type to ease test fixture creation
Exhaustive testing suite to validate permissions are applied as specified by business logic
Created a new test fixture with more production-like government roles to test finer-grained permissions
Refactored commenting permissions to allow client and server to share the same logic (the server passes allowable commenting actions as part of a credit trade API response)
Tooltips/disabled statuses for action buttons when commenting mode is active

Changelog: - Model created - Serializers created and adjuste - privileged_access boolean field to control API visibility of comments Todo: - Create new permission (VIEW|EDIT)_PRIVILIGED_COMMENT - Create route/viewSet to save comments - Wire up the frontend - Unit tests

Testing mostly finished now. A few more test cases to include for completeness. Pending: - Wire up the frontend - A handful of additional api test cases

API is now built and has thorough unit-testing

roles_permissions_v0.3.0.json

Comments API

Adjusted permission class to use `'key' in dict` format rather than dict.has_key('key'). Confirmed working with Python 3.6.4

Unit Testing Fix

Added front-end code to show Credit Balance of the selected organization for IDIR Users Added a check on the user's permission before showing the balance of the organization Updated test_api_custom so it follows pep8 coding style a bit better Added MinSerializer for Organization to reduce the amount of data (and add security) coming in for Credit Trades

…sions

Added Declined indicator in the breadcrumbs

Rescinded as a Flag, instead of Status

…SQL reserved word

# Conflicts: # backend/api/fixtures/test_credit_trade_comments.json # backend/api/models/CreditTrade.py # backend/api/models/CreditTradeComment.py # backend/api/models/CreditTradeHistory.py # backend/api/models/User.py # backend/api/permissions/CreditTradeComment.py # backend/api/serializers/CreditTrade.py # backend/api/serializers/CreditTradeHistory.py # backend/api/services/CreditTradeService.py # backend/api/test_api.py # backend/api/test_api_credit_trade_comment.py # backend/api/test_credit_trades.py # backend/api/viewsets/CreditTrade.py # backend/nose.cfg # frontend/src/credit_transfers/CreditTransferViewContainer.js # frontend/src/credit_transfers/components/CreditTransferDetails.js # frontend/src/credit_transfers/components/CreditTransferFormButtons.js # frontend/src/credit_transfers/components/CreditTransferProgress.js # frontend/src/credit_transfers/components/CreditTransferTable.js

…issions are assigned

plasticviking · 2018-06-29T18:30:46Z

frontend/src/credit_transfers/components/CreditTransferFormButtons.js

-        disabled={props.disabled.BTN_SIGN_2_2}
-        title={props.permissions.BTN_SIGN_2_2
+        disabled={props.isCommenting || props.disabled.BTN_SIGN_2_2}
+        title={props.isCommenting ? Lang.TEXT_COMMENT_DIRTY : (props.permissions.BTN_SIGN_2_2


I know that eslint will complain about the nested ternary, but it's the most concise way to do this, and the parens make precedence unambiguous.

…r-fuel-suppliers # Conflicts: # backend/api/serializers/CreditTrade.py

kuanfandevops · 2018-06-29T22:04:47Z

Do you have a Trello card for this?

plasticviking · 2018-06-29T22:10:48Z

Closes issue #395 (Trello card 688 https://trello.com/c/tGlVgY8P)

amichard · 2018-07-03T15:09:21Z

backend/api/migrations/0015_rename_comment.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-


Can you adjust the filename for this so it's incremented by 1? Just to avoid two 15's

amichard · 2018-07-03T15:35:08Z

Some notes I noticed while I was testing this:

During the review step, when I have the option to recommend and not recommend, I can't see the comment button. Am I missing some fixtures? (I can see it fine during the approval/decline step)

During the approval step, I noticed that there's no space between "Decline to Approve" and "Approve" buttons.

plasticviking · 2018-07-03T16:16:54Z

@amichard I'll check that out. It's something to do with the spacing being different when the button is in disabled state. I haven't added any special rules so I'll dig into it.

Regarding the buttons not appearing in review state, that's a business logic rule. Per Justin analysts should be able to comment when in signed 2/2 and reviewed and directors should only be able to comment when in reviewed.

I created test fixture users for my test cases that have either RECOMMEND or (DECLINE|APPROVE) credit transfer but not both, in order to test the logic.

- button styling - migration file names - also snuck in a column rename per comment on Trello card bcgov#688

plasticviking · 2018-07-03T17:24:36Z

@amichard All review comments addressed in latest push

amichard

Looks good!

This fixes the issue brought in by #414 Add commenting for fuel suppliers

…pliers. Changelog highlights: Added natural key lookups for User, Role, Permission, Credit Trade Status, and Type to ease test fixture creation Exhaustive testing suite to validate permissions are applied as specified by business logic Created a new test fixture with more production-like government roles to test finer-grained permissions Refactored commenting permissions to allow client and server to share the same logic (the server passes allowable commenting actions as part of a credit trade API response) Tooltips/disabled statuses for action buttons when commenting mode is active

This fixes the issue brought in by #414 Add commenting for fuel suppliers

plasticviking and others added 30 commits June 15, 2018 09:25

WIP

e04f946

Testing mostly finished now. A few more test cases to include for completeness. Pending: - Wire up the frontend - A handful of additional api test cases

Comments API

a60a113

API is now built and has thorough unit-testing

Split new permissions into release fixture

afc237b

roles_permissions_v0.3.0.json

Updated test to include new fixture

11029d2

Merge pull request bcgov#380 from plasticviking/comments-api

9a7dc14

Comments API

Python3 Compatibility Fix

692c496

Adjusted permission class to use `'key' in dict` format rather than dict.has_key('key'). Confirmed working with Python 3.6.4

Merge pull request bcgov#382 from plasticviking/comments-api

848257d

Unit Testing Fix

Added Declined indicator in the breadcrumbs

8744c64

Allow partial updates through PATCH. Rescinded should now be a flag

8e8a935

Updated breadcrumbs to show rescinded status. Added checks for permis…

bc9d733

…sions

Code cleanup

b3d5be8

Comments UI complete

e5cb123

Cleanup

dfdcb5e

Typo fix

9561506

Cleanup

57c6421

Minor refactoring. Added tests

db03195

Fixed tests

ee187a0

Merge pull request bcgov#386 from amichard/credit-trade-decline

e1e3c70

Added Declined indicator in the breadcrumbs

test site also uses logontest

7cd0a2d

Fixed conflicts

aa5f575

Renamed the migration table for credit trade comment

e5e721c

Merge pull request bcgov#389 from amichard/rescinded

e2faf35

Rescinded as a Flag, instead of Status

Refactored to address review comments

fe9770b

Merge conflict resolution

7dc5f53

Migrations fixed up

2f9cb63

Renamed the database column for the credit_trade_comment to avoid an …

2d3967b

…SQL reserved word

About to merge master to pickup upstream changes.

9eda0ac

plasticviking added 4 commits June 27, 2018 16:13

merge complete, fixtures updated, tests passing

f183837

about to merge with master

d2ff8c4

Fixed testing issue, reworked and simplified the the way comment perm…

bec2f95

…issions are assigned

Fuel suppliers commenting is now complete

5619cd4

plasticviking requested a review from amichard June 29, 2018 18:29

plasticviking commented Jun 29, 2018

View reviewed changes

plasticviking added this to the RELEASE v0.3.1 milestone Jun 29, 2018

Merge remote-tracking branch 'upstream/master' into add-commenting-fo…

5e8c31b

…r-fuel-suppliers # Conflicts: # backend/api/serializers/CreditTrade.py

fixed merge conflicts

e8ecbc9

amichard reviewed Jul 3, 2018

View reviewed changes

plasticviking added 2 commits July 3, 2018 10:20

Fixed review issues:

5a56ef0

- button styling - migration file names - also snuck in a column rename per comment on Trello card bcgov#688

tiny codestyle fix

81e1fc3

amichard approved these changes Jul 3, 2018

View reviewed changes

kuanfandevops merged commit 3df1803 into bcgov:master Jul 3, 2018

kuanfandevops pushed a commit that referenced this pull request Jul 3, 2018

Unit Testing Fix (Python Version issue) (#416)

6476e8e

This fixes the issue brought in by #414 Add commenting for fuel suppliers

kuanfandevops pushed a commit that referenced this pull request Sep 19, 2019

Unit Testing Fix (Python Version issue) (#416)

e70a468

This fixes the issue brought in by #414 Add commenting for fuel suppliers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commenting for fuel suppliers #414

Add commenting for fuel suppliers #414

plasticviking commented Jun 29, 2018

plasticviking Jun 29, 2018

kuanfandevops commented Jun 29, 2018

plasticviking commented Jun 29, 2018

amichard Jul 3, 2018

amichard commented Jul 3, 2018

plasticviking commented Jul 3, 2018 •

edited

Loading

plasticviking commented Jul 3, 2018

amichard left a comment

Add commenting for fuel suppliers #414

Add commenting for fuel suppliers #414

Conversation

plasticviking commented Jun 29, 2018

plasticviking Jun 29, 2018

Choose a reason for hiding this comment

kuanfandevops commented Jun 29, 2018

plasticviking commented Jun 29, 2018

amichard Jul 3, 2018

Choose a reason for hiding this comment

amichard commented Jul 3, 2018

plasticviking commented Jul 3, 2018 • edited Loading

plasticviking commented Jul 3, 2018

amichard left a comment

Choose a reason for hiding this comment

plasticviking commented Jul 3, 2018 •

edited

Loading