feat(account): add possibility to block #73
Conversation
Table added for blocking accounts.
Table policies and functions adjusted to exclude data of blocked accounts.
There was a problem hiding this comment.
Good work, the EXCEPT keyword is actually new to me!
In general, these changes definitely make tests necessary now, I'd say, as the complexity of all conditions grew to a point now where the overall meaning is not easily understandable at a glance anymore.
Would you fill some files in the verify folder please by adding some test INSERTs that mimic the most crucial use cases?
"A blocked B, so B's contact is not visible for A any more" as a simple example.
| DECLARE | ||
| jwt_account_id UUID; | ||
| BEGIN | ||
| jwt_account_id := NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID; | ||
|
|
||
| RETURN QUERY | ||
| SELECT invitation.event_id FROM maevsi.invitation | ||
| SELECT event_id FROM maevsi.invitation |
There was a problem hiding this comment.
In a few places it's necessary to keep this prefix or the column name would be ambiguous, but maybe this is not necessary here any more if you've checked it and it works without. If this function is confirmed to still be working, you can resolve this comment.
There was a problem hiding this comment.
For the future I recommend short aliases like here:
SELECT i.event_id FROM maevsi.invitation i
There was a problem hiding this comment.
All of maevsi's codebase tries to prevent using abbreviations if possible as each abbreviation adds to the knowledge required to build an understanding of the code. It's considered less work to read a word than to look for, remember and recall the meaning of an abbreviation, especially as this project should stay as welcoming as possible to new developers.
I see that this kind of abbreviation you suggest is often used. I just want to not have the future discussions with others about "if we use abbreviations here, why not in another place as well" that I'm sure I'll be having... So let's not use abbreviations if there is not a requirement for them. Do you see such a requirement?
| invitee_count_maximum IS NULL | ||
| OR | ||
| invitee_count_maximum > (maevsi.invitee_count(id)) -- Using the function here is required as there would otherwise be infinite recursion. | ||
| ) |
There was a problem hiding this comment.
Just fyi: changing the indentation while also having changes to the code makes it hard to read the difference for reviewers as you can see in above. The code stays mostly the same here, but there are way more green and red lines than necessary for an addition. Therefore, it's better to change formatting in a separate PR. You can resolve this comment once understood.
There was a problem hiding this comment.
What are the general formatting rules?
There was a problem hiding this comment.
I aim to have our code in the same formatting as the schema file output eventually. For now, the formatting should generally be kept as is in the existing files. Reformatting can happen though of course, just in a separate PR please.
In order to make account blocking work correctly (data of a blocked account must not be visible to the blocking account and vice versa) several fixes became necessary. In addition some code formatting issues were handled.
sthelemann
force-pushed
the
feat/account_block
branch
from
4ff342c to
c55cd8f
Compare
schema/schema.definition.sql
Outdated
| @@ -993,8 +993,28 @@ COMMENT ON FUNCTION maevsi.events_organized() IS 'Add a function that returns al | |||
| CREATE FUNCTION maevsi.invitation_claim_array() RETURNS uuid[] | |||
| LANGUAGE plpgsql STABLE STRICT | |||
| AS $$ | |||
| DECLARE | |||
There was a problem hiding this comment.
(just using this first line out of context)
Blocking must be added to the table event_category_mapping_policy:
There was a problem hiding this comment.
@sthelemann this still needs to be implemented
Test cases for account blocking are implemented in file(s) test_account_blocking.sql. Several convenience test functions are created in the new schema maevsi_test.
|
My original intention to insert the tests into verify/table_account_block.sql didn't work. I kept running into circular dependencies when creating schema.definition.sql, so I created a new set of files test_account_blocking.sql and put them at the end of sqitch.plan. I think it is better anyway to separate (complex) test cases from a single table creation. |
dargmuesli
force-pushed
the
feat/account_block
branch
from
bf4d2c1 to
c9aa441
Compare
The policy for table event_category_mapping has been updated and tested.
Table added for blocking accounts.