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

Accepting invitation #982

Merged
merged 17 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@Macroz
Copy link
Collaborator

Macroz commented Mar 1, 2019

Implements accept invitation coming through a join link from #609

Macroz added some commits Feb 20, 2019

refactor: remove optionality
Basically you should always set at least the :title
fix: formatting of errors in status-modal
- into :div to prevent react keyless warnings
- don't force content into strings
refactor: helpers for better assert messages
Using ex-info here allows passing the actual error to the test in the exception
Show resolved Hide resolved resources/sql/queries.sql Outdated
Show resolved Hide resolved resources/translations/en.edn
Show resolved Hide resolved src/clj/rems/api/applications.clj
@@ -510,7 +514,8 @@
(defn- hide-sensitive-information [application]

This comment has been minimized.

@opqdonut

opqdonut Mar 4, 2019

Collaborator

hmm why do we have this and the rems.api.applicaitons/hide-sensitive-information functions?

This comment has been minimized.

@luontola

luontola Mar 4, 2019

Collaborator

They are v1 and v2 versions of the API.

This comment has been minimized.

@luontola

luontola Mar 4, 2019

Collaborator

Though I'm confused that how :invitation-tokens finds its way to the v2 API so that it would need to be removed here. AFAIK, only v1 code is setting :invitation-tokens. Is this dissoc dead code?

This comment has been minimized.

@Macroz

Macroz Mar 5, 2019

Author Collaborator

I'm at a loss about what should work and where. Seems like v2 is full of placeholders in event-type-specific-application-view?

:invitation-tokens is set by the code in the dynamic.clj but if that's not used in the future and if someone will later write the correct specific views then this line is obviously not needed.

Show resolved Hide resolved src/clj/rems/db/applications.clj
Show resolved Hide resolved src/clj/rems/workflow/dynamic.clj
Show resolved Hide resolved src/clj/rems/workflow/dynamic.clj Outdated
(:members
(apply-commands application
[{:type ::accept-invitation :actor "somebody" :token "very-secure"}]
injections))))))))

This comment has been minimized.

@opqdonut

opqdonut Mar 4, 2019

Collaborator

how about an accepted application?

This comment has been minimized.

@Macroz

Macroz Mar 5, 2019

Author Collaborator

I didn't intend to test all the possible states where it's possible to join. It's more significant at least that a submitted application can be joined since a draft is already checked. Should I test all the states here?

This comment has been minimized.

@opqdonut

opqdonut Mar 5, 2019

Collaborator

I'd like at least one negative case where the state of the application is the reason for the failure (instead of invalid code or missing invitation), but I'm not insisting.

This comment has been minimized.

@Macroz

Macroz Mar 5, 2019

Author Collaborator

Well accepting the invitation should always be possible but maybe not when the application has been closed already. That also creates a new situation that has to be shown to the joiner so maybe I'll add that one.

Show resolved Hide resolved src/cljs/rems/actions/accept_invitation.cljs Outdated
Show resolved Hide resolved src/cljs/rems/spa.cljs
@opqdonut
Copy link
Collaborator

opqdonut left a comment

feel free to merge, or add the test

Macroz added some commits Mar 5, 2019

refactor: add :everyone-else role
- remove has-any-role? that doesn't really work
- introduce see-application? helper for specific use
- test that :everyone-else can join application unless closed
- fix add-member tests
- return more specific error than :forbidden if there is one available
  I.e. must return already-member instead for better user experience

@Macroz Macroz merged commit 121a23f into master Mar 5, 2019

6 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cloverage 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: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details

@Macroz Macroz deleted the invitations branch Mar 5, 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.