Skip to content
This repository has been archived by the owner on Sep 22, 2021. It is now read-only.

Using council members from chain db rather than onchain. #814

Merged
merged 91 commits into from Jun 10, 2020

Conversation

niklabh
Copy link
Contributor

@niklabh niklabh commented May 27, 2020

Showing old council if poll closing time has passed

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

A first round of comments regarding the DB:

  • I think users should not be able to update the post_id of a poll. Nor should anyone (except admin ofc) be able to delete a poll.

image

  • There's no trigger for both the poll and poll_votes tables. This is needed for the updated_at column otherwise it will never be updated.
    image

To create a trigger, first delete this column (with a SQL DROP COLUMN .. CASCADE because I think the UI won't let you do it. and create it again using the "frequently used column button". This triggers updates the field automatically then.
image

  • The post table has no link to polls. We should create it by clicking on this button. I haven't looked at the code but something tells me that we're doing an unneeded additional call in the front, although we could simply add the poll to the post query (and if post.poll !== null then we show the poll)
    image

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Let me know if my suggestion isn't clear, or super annoying.
The advantage is to have 1 single query for posts, with everything. No additional queries for poll display or poll creation. the disadvantage are:

  • a monster query (with fragments it should be manageable)
  • it adds some props drilling
  • re-fetching is annoying

@@ -55,37 +60,64 @@ const CreatePost = ({ className }:Props): JSX.Element => {
.catch((e) => console.error('Error subscribing to post',e));
};

const createPoll = (postId: number) => {
Copy link
Collaborator

@Tbaut Tbaut Jun 10, 2020

Choose a reason for hiding this comment

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

this additionnal createPoll is not needed if you link the post to polls in hasura. You can create a nested mutation elegantly by creating the poll at the same time as the post.

@niklabh
Copy link
Contributor Author

niklabh commented Jun 10, 2020

Let me know if my suggestion isn't clear, or super annoying.
The advantage is to have 1 single query for posts, with everything. No additional queries for poll display or poll creation. the disadvantage are:

  • a monster query (with fragments it should be manageable)
  • it adds some props drilling
  • re-fetching is annoying

I am in favour of seperate queries. So that post and polls load seperately. If a post has so many poll votes the whole page will load slow. If queries are seperate post will load and poll sidebar will render later making time to meaningful render fast.

Let me know what do you think.

@Tbaut
Copy link
Collaborator

Tbaut commented Jun 10, 2020

Yes, that's totally fair. This would make the code harder to read/debug.
Let's keep the queries separated for the post/poll loading.

I think for the creation though, it'd be much better if we can create the post and the poll in 1 query.

@niklabh
Copy link
Contributor Author

niklabh commented Jun 10, 2020

Yes, that's totally fair. This would make the code harder to read/debug.
Let's keep the queries separated for the post/poll loading.

I think for the creation though, it'd be much better if we can create the post and the poll in 1 query.

For creation i am checking

@niklabh
Copy link
Contributor Author

niklabh commented Jun 10, 2020

Yes, that's totally fair. This would make the code harder to read/debug.
Let's keep the queries separated for the post/poll loading.

I think for the creation though, it'd be much better if we can create the post and the poll in 1 query.

We want conditional poll creation if hasPoll is checked. We cannot do it in one query with graphql. It will create a poll row each time.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Looks all very good, thank you so much for this feature. I've added a PR to prevent anyone from voting once the end of the poll has been reached.
#878

@Tbaut Tbaut merged commit 66564f0 into master Jun 10, 2020
@Tbaut Tbaut deleted the block-number-check branch June 10, 2020 15:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants