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

Feature add git workflow for slr search #7625

Merged
merged 16 commits into from
Jul 4, 2021
Merged

Feature add git workflow for slr search #7625

merged 16 commits into from
Jul 4, 2021

Conversation

DominikVoigt
Copy link
Contributor

This PR adds Toolbar actions for starting a new SLR and continuing an existing SLR.
Besides the documentation this PR is complete.
I will create a documentation parallel to this one sometime this week.

Toolbar entries:
image

Starting new study UI:
image

TODO: Add & Update documentation: JabRef/user-documentation#333

  • 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.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 15, 2021
calixtus
calixtus requested changes Apr 16, 2021
View reviewed changes
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

After a very quick first look good work, yet some smaller issues with the mvvm pattern.

src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java
}

@FXML
public void selectStudyDirectory() {
Copy link
Member

Choose a reason for hiding this comment

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

Logic should go to the viewmodel

src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
tobiasdiez
tobiasdiez reviewed Apr 16, 2021
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

LGTM.

Is it possible to make git management opt-in? I think most of our users don't use git.

src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
@koppor
Copy link
Member

koppor commented May 2, 2021

Is it possible to make git management opt-in? I think most of our users don't use git.

Maybe, we can just omit the "git push" thing. And the other things should "just work", because jgit is used instead of a local git configuration. - We opted for using git, because it provides us a history of the searches out of the box. We did not want to implement a new version-control system. 😇

@koppor koppor added status: changes required Pull requests that are not yet complete and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 2, 2021
@calixtus calixtus self-assigned this May 24, 2021
calixtus
calixtus requested changes May 29, 2021
View reviewed changes
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Took a deeper look into your code. Most of it looks good though i have some remarks on code style.

src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinitionView.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/StudyDatabaseItem.java Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/main/java/org/jabref/gui/slr/ManageStudyDefinition.fxml Outdated Show resolved Hide resolved
@calixtus calixtus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels Jun 17, 2021
calixtus and others added 2 commits June 18, 2021 03:13
@calixtus
Reduced code duplication
9b17012
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into feature-add-git-wor…
bf9f201 
…kflow-for-slr-search

* upstream/main: (31 commits)
  New translations JabRef_en.properties (German) (#7868)
  Fix test "higherTrustLevelWins()" (#7866)
  Change WM_CLASS to jabref (#7858)
  [Bot] Update CSL styles (#7865)
  Add unit test to four test classes (#7651)
  Fix IEEE test (#7852)
  New Crowdin updates (#7859)
  Fix markdown syntax of ADRs
  add missing l10n (#7857)
  New Crowdin updates (#7847)
  Bump mockito-core from 3.11.1 to 3.11.2 (#7856)
  Bump checkstyle from 8.43 to 8.44 (#7855)
  Fix for issue #4652: Add Find Unlinked Files Filter based on Date (#7846)
  Fix for entering a backslash in the custom entry preview dialog (#7851)
  Fixed INSPIREFetcherTest
  Fixed TitleFetcherTest
  Ignore baeldung.com and tldrlegal.com from out link checks
  New Crowdin updates (#7845)
  New Crowdin updates (#7843)
  Refactoring and addition of unit tests (#7597)
  ...

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
@Siedlerchr Siedlerchr merged commit f0709b7 into main Jul 4, 2021
@Siedlerchr Siedlerchr deleted the feature-add-git-workflow-for-slr-search branch July 4, 2021 19:02
@koppor koppor mentioned this pull request Jul 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@tobiasdiez tobiasdiez tobiasdiez left review comments

@calixtus calixtus calixtus requested changes

Assignees

@calixtus calixtus

Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DominikVoigt @koppor @Siedlerchr @tobiasdiez @calixtus