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

Change approved resources #1227

Merged
merged 22 commits into from May 13, 2019

Conversation

Projects
None yet
2 participants
@Macroz
Copy link
Collaborator

commented May 9, 2019

Closes #1104

I had to rewrite the entitlement giving logic, but fortunately we have some coverage in the acceptance tests and I added tests for changing resources there.

There is possibly some refactoring opportunity still left.

Macroz added some commits May 3, 2019

@opqdonut
Copy link
Collaborator

left a comment

looks good, some golfing

Show resolved Hide resolved resources/sql/queries.sql
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved test/clj/rems/db/test_entitlements.clj Outdated
@opqdonut
Copy link
Collaborator

left a comment

some suggestions. fine with merging.

Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
Show resolved Hide resolved src/clj/rems/db/entitlements.clj Outdated
members-to-update (set
(concat (keys entitlements-to-add)
(keys entitlements-to-remove)))]
(when (seq members-to-update)

This comment has been minimized.

Copy link
@opqdonut

opqdonut May 13, 2019

Collaborator

hmm if you split out the computation of entitlements-to-{add,remove} you could write unit tests for it

This comment has been minimized.

Copy link
@Macroz

Macroz May 13, 2019

Author Collaborator

I can't see an easy way to split them out that would not duplicate the fetching logic. I'll leave this for later PRs.

@@ -25,7 +25,7 @@
(throw (InterruptedException.)))
(try
;; TODO: add proper monitoring for pollers and caches
(log/info name-kw "processing event" (:event/id event))
(log/info name-kw "processing event" (:event/id event) (:event/type event))

This comment has been minimized.

Copy link
@opqdonut

opqdonut May 13, 2019

Collaborator

consider (select-keys event [:event/id :event/type])

This comment has been minimized.

Copy link
@Macroz

Macroz May 13, 2019

Author Collaborator

There is no guaranteed order in logs then?

This comment has been minimized.

Copy link
@opqdonut

opqdonut May 13, 2019

Collaborator

ok good point

@Macroz Macroz merged commit cb0f297 into master May 13, 2019

7 checks passed

WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details

@Macroz Macroz deleted the change-approved-resources branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.