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.

Sign up for GitHub

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

Jump to bottom

migration of timestamp #7671

Merged
merged 14 commits into from
May 1, 2021
Merged

migration of timestamp #7671

merged 14 commits into from
May 1, 2021

Conversation

DominikVoigt
Copy link
Contributor

This PR moves the migration of the timestamp field from a PostOpenMigration to mutually exclusive cleanup actions.
As this cleanup action migrates the timestamp field to the creationdate or modificationdate fields,
the modificationdate of a BibEntry is not updated when this cleanup is conducted.
If the original timestamp field could not be parsed, the cleanup does not modify the BibEntry in any way to not lose data.
Fixes #7351.

Modified Cleanup dialog:
image

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@tobiasdiez
Copy link
Member

On a first look, this looks good to me. A few observations though:

  • The UI is a bit too complicated for my taste and I think the "Do not convert timestamp" option is not really required. Instead I would propose to only have to checkboxes (convert to creation date / modification date), and if the user selects one of them the other one is deselected (that would be btw also a nice improvement for the biblatex/bibtex conversion which are also mutable exclusive)
  • Now running cleanup ops doesn't change the modification date anymore, right? Maybe use a more fine-grained notification channel and only do this for the timestamp cleanup?
  • Can you maybe explore a bit on why you choose a cleanup op over a migration? Thanks

@Siedlerchr Siedlerchr changed the title Fix/7351 migration of timestamp Apr 26, 2021
@DominikVoigt
Copy link
Contributor Author

On a first look, this looks good to me. A few observations though:

  • The UI is a bit too complicated for my taste and I think the "Do not convert timestamp" option is not really required. Instead I would propose to only have to checkboxes (convert to creation date / modification date), and if the user selects one of them the other one is deselected (that would be btw also a nice improvement for the biblatex/bibtex conversion which are also mutable exclusive)

I addressed the first point by adding checkboxes for the corresponding cleanup actions.

  • Now running cleanup ops doesn't change the modification date anymore, right? Maybe use a more fine-grained notification channel and only do this for the timestamp cleanup?

This is only true for the timestamp migrations and is implemented exactly that way ^^

  • Can you maybe explore a bit on why you choose a cleanup op over a migration? Thanks

As an ADR or just as a comment here?
It boils down to the discussion lead here: #7351.

@tobiasdiez
Copy link
Member

On a first look, this looks good to me. A few observations though:

  • The UI is a bit too complicated for my taste and I think the "Do not convert timestamp" option is not really required. Instead I would propose to only have to checkboxes (convert to creation date / modification date), and if the user selects one of them the other one is deselected (that would be btw also a nice improvement for the biblatex/bibtex conversion which are also mutable exclusive)

I addressed the first point by adding checkboxes for the corresponding cleanup actions.

Thanks, that looks good to me!

  • Now running cleanup ops doesn't change the modification date anymore, right? Maybe use a more fine-grained notification channel and only do this for the timestamp cleanup?

This is only true for the timestamp migrations and is implemented exactly that way ^^

Ohh, you are right. Sorry, I misread the code.

  • Can you maybe explore a bit on why you choose a cleanup op over a migration? Thanks

As an ADR or just as a comment here?
It boils down to the discussion lead here: #7351.

As a comment is more than enough for me (maybe directly in the PR description though). As far as I see it, the advantage of cleanup ops is that they are only run once and thus don't make the db load process slower; on the other hand the migration had the advantage that user's don't need to run something manually.

@DominikVoigt
Copy link
Contributor Author

The reason for making it a cleanup is that some users use the timestamp field in other ways.
Additionally, this makes the database load process faster.
Migrating the timestamp field this way leads to negative UX see #7351.

@tobiasdiez
Copy link
Member

Ok thanks!

I don't have the time for a through-out review, but scrolling over the code I couldn't find anything. So from my side this is good to go.

@koppor koppor merged commit 093bac2 into main May 1, 2021
@koppor koppor deleted the fix/7351 branch May 1, 2021 16:06
Siedlerchr added a commit that referenced this pull request May 4, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into udpatejaxb
4c8f270 
* upstream/main: (354 commits)
  Fix ScienceDirect fetcher (#7684)
  Refactor NoBibTexFieldCheckerTest to increase mutation coverage (#7697)
  Update Gradle from 6.8.3 to 7.0 (#7619)
  Fixes #7305: the RFC fetcher is not compatible with the draftFix for issue 7305 (#7674)
  Refactoring existing unit tests (#7693)
  cover boundary cases & add more unit tests (#7694)
  Bump classgraph from 4.8.104 to 4.8.105 (#7688)
  Bump java-diff-utils from 4.9 to 4.10 (#7692)
  Fix arXiv fetcher tests (#7686)
  Make key for ScienceDirect configurable (#7683)
  migration of timestamp (#7671)
  Fix CCSB and DOAJ (#7426)
  [Bot] Update CSL styles (#7680)
  MS Office XML: Export month name (#7677)
  linkfix (#7678)
  readd fix (#7675)
  Fix threading cleanup in performSearch (#7672)
  add missing changelog
  delete bug fix (#7580)
  Add more unit tests to three gui classes  (#7636)
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@DominikVoigt @tobiasdiez @koppor @calixtus