Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

feat: add instance settings to schema #2425

Closed
wants to merge 24 commits into from
Closed

feat: add instance settings to schema #2425

wants to merge 24 commits into from

Conversation

Sboonny
Copy link
Member

@Sboonny Sboonny commented Feb 28, 2023 •
edited
Loading

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the main branch of Chapter.

Closes #2408

@ghost
Copy link

ghost commented Feb 28, 2023 •
edited by ghost
Loading

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Sboonny Sboonny force-pushed the feat/add-dashboard-to-change-chapter-setting branch from c3c009e to d510d04 Compare February 28, 2023 16:41
@Sboonny @gikf
generate the API CALL
00d6277 
Co-authored-by: gikf <60067306+gikf@users.noreply.github.com>
@Sboonny Sboonny force-pushed the feat/add-dashboard-to-change-chapter-setting branch from d510d04 to 00d6277 Compare February 28, 2023 16:51
@Sboonny Sboonny changed the title feat: add chapter setting to dashboard feat: add instance setting to dashboard Mar 1, 2023
@Sboonny Sboonny mentioned this pull request Mar 13, 2023
Closed
3 tasks
@Sboonny Sboonny force-pushed the feat/add-dashboard-to-change-chapter-setting branch from 09fea3e to 1edbfe4 Compare March 13, 2023 06:46
@Sboonny Sboonny marked this pull request as ready for review March 24, 2023 13:01
@Sboonny Sboonny requested a review from a team March 24, 2023 13:01
@gikf gikf changed the title feat: add instance setting to dashboard feat: add instance setting to schema Mar 27, 2023
@gikf gikf changed the title feat: add instance setting to schema feat: add instance settings to schema Mar 27, 2023
gikf
gikf reviewed Mar 27, 2023
View reviewed changes
@Sboonny @gikf
Remove increasing id 😅
d3c2bcf 
Co-authored-by: Krzysztof G. <60067306+gikf@users.noreply.github.com>
@Sboonny Sboonny marked this pull request as draft March 27, 2023 10:18
@Sboonny @ojeytonwilliams
Fix schema and remove extra id
19efa78 
Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
@Sboonny Sboonny marked this pull request as ready for review March 27, 2023 14:52
@ojeytonwilliams
Copy link
Contributor

Sorry, @Sboonny, I didn't see @gikf's comment (and your conversation) when I made my suggestion.

While there are some things we can do to ensure that the table has one record, I would just go with @gikf's approach, but with a comment on the migration to explain why we're not generating ids automatically.

ojeytonwilliams
ojeytonwilliams suggested changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Thanks for adding the comment @Sboonny, it helped me understand the requirements. Ironically, that means we don't need it!

@Sboonny @ojeytonwilliams
Remove the comment
f336c5d 
Co-authored-by: Oliver Eyton-Williams <ojeytonwilliams@gmail.com>
@ojeytonwilliams
Copy link
Contributor

Sorry for the delay on this one, but I think it's a bad idea to update the database without including the code that makes use of those updates. Reason being I can't follow the logic about how this is used. Also, more generally each PR should "do one thing", but this PR "does half a thing". I hope that makes sense!

Could you combine this PR with #2467 ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gikf gikf gikf left review comments

@ojeytonwilliams ojeytonwilliams ojeytonwilliams requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Self-hosted privacy policy, terms of service and code of conduct
3 participants
@Sboonny @ojeytonwilliams @gikf