Skip to content

Fix import of MS Office xml in case of invalid month#2538

Merged
stefan-kolb merged 4 commits into
masterfrom
msxmlimport
Feb 17, 2017
Merged

Fix import of MS Office xml in case of invalid month#2538
stefan-kolb merged 4 commits into
masterfrom
msxmlimport

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Feb 12, 2017

Copy link
Copy Markdown
Member

Fix for: http://discourse.jabref.org/t/msbib-import-not-working/463
Fix month field.shortname returning null in case of invalid Month

  • Change in CHANGELOG.md described
  • Tests created for changes
    - [ ] Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
    - [ ] Check documentation status (Issue created for outdated help page at [help.jabref.org](https://github.com/JabRef/help.jabref.org/issues)?)
    - [ ] If you changed the localization: Did you run gradle localizationUpdate?

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 12, 2017
Month month = MonthUtil.getMonth(entry.month);
fieldValues.put(FieldName.MONTH, month.shortName);
//if we encouter an UnknownMonth/NULL_OBJECT month, the shortname returns null
if (month.shortName == null) {

@tobiasdiez tobiasdiez Feb 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is cleaner if you modify the getMonth method to return Optional<Month>̀ instead of NULL objects.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah. Thought about remodeling the stuff. The NULL_OBJECT stuff and UnknownMonth is really shitty,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use the isValid method of a Month to determine whether it is a valid month.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done and added also test

@stefan-kolb

Copy link
Copy Markdown
Member

Can you please add a test for this if possible?

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 13, 2017
* upstream/master: (182 commits)
  Reorder pferences tab to be more intuitive (#2560)
  Reoder translations to be more meaningful
  move jmh benchmark
  fix prefs import test
  Add prefs migration
  add missing files
  replace text occurrences
  fix logging
  fix imports
  fix intellij code style
  fix jaxb based importers
  fix  search generation
  move and fix tests
  fix compilation errors
  update checkstyle for correct positioning of org.jabref packages
  fix wrong positioning of some gui classes
  fix generation of sources
  second part of gui
  first part of gui
  second part of logic move
  ...

# Conflicts:
#	CHANGELOG.md
Add test for importer
@Siedlerchr

Copy link
Copy Markdown
Member Author

Check for valid month and added test

@stefan-kolb stefan-kolb merged commit 62514b6 into master Feb 17, 2017
@stefan-kolb stefan-kolb deleted the msxmlimport branch February 17, 2017 16:16
Siedlerchr added a commit that referenced this pull request Feb 18, 2017
* upstream/master:
  Fix colour of tooltip
  Groups: click on arrow should toggle expansion status
  Fix import of MS Office xml in case of invalid month (#2538)
  Change MainClass from net.sf to org.jabref.JabRefMain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants