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

Fix seeds overriding edited help articles #1175

Merged
merged 2 commits into from Sep 14, 2023

Conversation

Taeir
Copy link
Contributor

@Taeir Taeir commented Aug 15, 2023

It seems that help articles don't (always) have an initial revision. Because the seeds were expecting them to have this, they could actually consider help articles that were edited as not-edited, and update them when reseeding...

It seems that help articles don't (always) have an initial revision.
Because the seeds were expecting them to have this, they could actually
consider help articles that were edited as not-edited, and update
them...
@cellio cellio requested a review from a team August 15, 2023 17:21
Copy link
Member

@Oaphi Oaphi left a comment

Choose a reason for hiding this comment

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

As for the change itself - LGTM to me, checked locally. I took the liberty to bring the branch up-to-date with develop, hopefully this is fine with you, @Taeir. I have one "elephant in the room" question, though - is there a reason why we don't have an initial revision for seeded policy posts?

@Oaphi Oaphi added the area: ruby Changes to server-side code label Sep 10, 2023
@Taeir
Copy link
Contributor Author

Taeir commented Sep 14, 2023

@Oaphi If I knew why, I would have fixed that instead :)

Regardless, I think it is good to do this type of "defensive programming" and not make too many assumptions on how data looks, such that it works in all cases.

@Taeir Taeir merged commit 35a3b0a into develop Sep 14, 2023
7 checks passed
@Taeir Taeir deleted the fix-seeds-overwriting-help-articles branch September 14, 2023 10:08
@Oaphi
Copy link
Member

Oaphi commented Sep 14, 2023

@Oaphi If I knew why, I would have fixed that instead :)

Regardless, I think it is good to do this type of "defensive programming" and not make too many assumptions on how data looks, such that it works in all cases.

Yup, no issue with the update, was just wondering if that's intended. I have a suspicion that the revision creation fails validation, but haven't got the time to look into it properly yet. Will let you know if that's the source of the problem!

@Oaphi
Copy link
Member

Oaphi commented Sep 14, 2023

@Taeir ah, yes, the PostHistory creation in the create_initial_revision callback on after_create fails due to the "User must exist" error and is silently swallowed. I will take a look when I am able to.

@Oaphi Oaphi removed the request for review from a team September 14, 2023 23:18
@Taeir
Copy link
Contributor Author

Taeir commented Sep 15, 2023

@Oaphi may be a seed order issue. I expect users to not have been created perhaps?

@Oaphi
Copy link
Member

Oaphi commented Sep 16, 2023

@Oaphi may be a seed order issue. I expect users to not have been created perhaps?

@Taeir nah, I've been running seeds on an already fully initialized database - it's actually due to the user not being passed as one of the parameters during post creation. I think if we just make the system user the owner of those posts, it should be fine. Will make a separate PR improving the seeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ruby Changes to server-side code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants