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

On #736: Added cleanup migrations #762

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

lentschi
Copy link
Contributor

(squashed and modified commits from issue_736)

Copy link
Member

@paroga paroga left a comment

Choose a reason for hiding this comment

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

THX, first check LGTM. i'll give it a deep check in the next days and try it on some database to check if there are issues

@paroga paroga force-pushed the master branch 6 times, most recently from ca76347 to c6250de Compare September 5, 2020 15:00
@lentschi lentschi mentioned this pull request Feb 5, 2021
@lentschi
Copy link
Contributor Author

lentschi commented Feb 12, 2021

I just fixed the unit tests by creating temporary tables for shared_articles and shared_suppliers. They aren't real temporary tables, but that I couldn't get to work with ActiveRecord for some reason.
I still believe it's cleaner than the current way of 'recycling' the internal articles and suppliers tables for testing though. (They have a completely different structure after all.)

The cleanest overall solution (apart from test refactoring) would be to use an API for this kind of import/export (or at least some sort of shared schema via a gem), but that's outside of the scope of this PR.

@yksflip yksflip added this to the 5.0 milestone Mar 11, 2024
@yksflip yksflip added the tech debt technical debt label Mar 11, 2024
@RayOei
Copy link

RayOei commented Mar 9, 2025

What is the intent here as is still in Draft and has been for some time 😇
Added clear link to #736 for navigation.

@lentschi
Copy link
Contributor Author

What is the intent here as is still in Draft and has been for some time

I'm not sure, what you're asking...

  • "Why is it still in Draft and not being merged?" -> Because we didn't dare to merge it back then as we didn't have the ressources to review it thoroughly and it might lead to the loss of data. (Actually I later found a few of the foreign key constraints I made here are simply wrong, but I didn't want to push this through on my own, so I hadn't invested time in correcting them.)
  • "Why is it still in Draft and not Closed?" -> Because the code might still be useful. Now that we have a bigger team, we might have another go at it. (But after Article versioning, extended units and alternative article sharing (#1058) #1073 we'll have to rewrite a lot of it, of course.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt technical debt
Projects
Status: Prioritised
Development

Successfully merging this pull request may close these issues.

4 participants