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

feat(flag proposal): add ability to flag proposal #679

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

Todmy
Copy link
Contributor

@Todmy Todmy commented Sep 21, 2023

Changes proposed in this pull request:

  • Fix query builder to get flagged/not flagged proposals
  • modified graphql helper
  • fix SQL schema

TODO:

  • Create a new column with
ALTER TABLE proposals 
ADD COLUMN flagged INT NOT NULL DEFAULT 0,
ADD INDEX flagged (flagged);

Notes:
This PR doesn't remove the previous implementation and is backward compatible. The previous implementation will be removed when data is migrated from the laser DB to the new column. Probably it will be done in a separate PR

Sorry, something went wrong.

@@ -66,13 +66,15 @@ export default async function (parent, args) {
params.push(verifiedSpaces);
}

// TODO: remove part `p.id IN (?)` when flagged proposals are moved from laser DB to snapshot-sequencer DB
if (where.flagged === true && flaggedProposals.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware of the case where this condition will not be triggered if flaggedProposals.length is 0.

Refactoring to remove p.id IN (?) has to be done before disconnecting laser database

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, thank you 🙏

wa0x6e
wa0x6e previously approved these changes Sep 24, 2023
Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

tAck

@wa0x6e wa0x6e dismissed their stale review September 24, 2023 16:08

typo error remaining

@@ -58,6 +59,7 @@ CREATE TABLE proposals (
INDEX scores_state (scores_state),
INDEX scores_updated (scores_updated),
INDEX votes (votes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing trailing comma here

Copy link
Contributor Author

@Todmy Todmy left a comment

Choose a reason for hiding this comment

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

.

@Todmy Todmy requested a review from wa0x6e September 25, 2023 09:38
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (e087e76) 0.00% compared to head (91bdbbd) 0.00%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #679   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          39      39           
  Lines        1878    1885    +7     
  Branches       39      39           
======================================
- Misses       1839    1846    +7     
  Partials       39      39           
Files Changed Coverage Δ
src/graphql/helpers.ts 0.00% <0.00%> (ø)
src/graphql/operations/proposals.ts 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@wa0x6e wa0x6e left a comment

Choose a reason for hiding this comment

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

tAck

Copy link
Member

@ChaituVR ChaituVR left a comment

Choose a reason for hiding this comment

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

utAck

@ChaituVR ChaituVR merged commit 928a0e2 into master Sep 28, 2023
@ChaituVR ChaituVR deleted the todmy/flag-proposal branch September 28, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants