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

Adding UI for publication workflow #1620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Adding UI for publication workflow #1620

wants to merge 2 commits into from

Conversation

SarahBA
Copy link
Contributor

@SarahBA SarahBA commented Dec 31, 2017

No description provided.

@emanueldima
Copy link
Contributor

@SarahBA can you attach some screenshots to this PR?

emanueldima
emanueldima reviewed Jan 4, 2018
View reviewed changes
webui/src/components/editfiles.jsx Outdated
@@ -266,6 +266,10 @@ export const EditFiles = React.createClass({
}
},

componentWillReceiveProps(props) {
return props.readOnly !== this.props.readOnly;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this does, I can't find any docs about a return value from componentWillReceiveProps, can you help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I was going to use shouldComponentUpdate but there were some issue that I can't recall now, so I decided to use componentWillReceiveProps instead and some how got confused! I'll check it again.

webui/src/components/editrecord.jsx Outdated
@@ -99,6 +99,9 @@ const EditRecord = React.createClass({
errors: {},
dirty: false,
waitingForServer: false,
readOnly: false,
checkboxValue: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

not a good name, which checkbox? is it about the confirmation for editing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The checkbox at the end of the edit draft. When it's true then the draft will be either sent for review or it's ready for publication. Maybe review_or_publish?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I would call it reviewOrPublishConfirmed

webui/src/components/editrecord.jsx Outdated
@@ -99,6 +99,9 @@ const EditRecord = React.createClass({
errors: {},
dirty: false,
waitingForServer: false,
readOnly: false,
checkboxValue: null,
revoking:false,
Copy link
Contributor

Choose a reason for hiding this comment

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

would help to add another word to this name, about what is being revoked -- I'm missing the context now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revoking_submitted_draft? Or revoking_submitted_for_review?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, revokeSubmitted or even unsubmit, your choice here

webui/src/components/editrecord.jsx Outdated
} else if (this.state.record && props.blockSchemas) {
const record = addEmptyMetadataBlocks(this.state.record, props.blockSchemas);
if (record) {
this.setState({record});
}
}

// Record is submitted for review: the publication_state is 'submitted' and readonly should be True
if(this.props.community && this.state.record){
if(this.props.community.getIn(["publication_workflow"]) == 'review_and_publish' && this.state.record.get('publication_state')=='submitted' && !this.state.revoking){
Copy link
Contributor

Choose a reason for hiding this comment

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

this.props.community.getIn(["publication_workflow"]) -> this.props.community.get("publication_workflow")

webui/src/components/editrecord.jsx Outdated
const original = this.props.record.get('metadata').toJS();
let updated = this.state.record.toJS();
// checkbox is enabled and record is going to be "submitted" for review by community admin
if (prevState.record.getIn(['publication_state']) !== this.state.record.getIn(['publication_state']) && !this.state.revoking){
Copy link
Contributor

Choose a reason for hiding this comment

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

you can replace getIn with get here

webui/src/components/editrecord.jsx Outdated
}
},

applyPatch(patch, caseValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good function structure, but the name doesn't show an important part of its function: changing the browser state. I'd rename it to something like applyPatchAndUpdateBrowser (this is not a great name, maybe you can find a better one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try ;)

webui/src/components/editrecord.jsx
<button type="submit" className={"btn btn-default btn-block "+klass} onClick={this.updateRecord}>{text}</button>
</div>
);
if(this.props.community){
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer an error here: if the community is not defined, the user cannot do anything anyway (the buttons are not rendered). This should not happen 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will fix it

webui/src/components/editrecord.jsx Outdated
</div>
);
} else {
const klass = this.state.waitingForServer ? 'disabled' :
Copy link
Contributor

Choose a reason for hiding this comment

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

I would try to merge these two branches and show the different texts depending on the state.

webui/src/components/editrecord.jsx Outdated
<button type="submit" className={"btn btn-default btn-block btn-primary btn-danger "} onClick={this.editSubmittedRecord}>Edit</button>
</div>
</div>
:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this better? move to separate functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. You mean moving the condition part to a separate function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of moving the divs into separate functions, but that may make the code even more difficult to follow. Let's leave it for the moment.

@SarahBA
Copy link
Contributor Author

SarahBA commented Jan 5, 2018 •
edited
Loading

create a new draft

1_create a new draft

save changes to the draft

2_save changes to the draft

ready to submit for review

3_ready to submit for review

submitted for review

4_submitted for review

editing and getting back to the draft mode

5-editing and getting back to the draft mode

user profile

6_user profile

search

7_search

@emanueldima
Copy link
Contributor

Thanks, it looks very nice!

@SarahBA SarahBA force-pushed the workflow branch 2 times, most recently from 76e7fcc to 85b7afb Compare February 5, 2018 13:11
@emanueldima
Copy link
Contributor

The PR looks good to me, but since I didn't test it and it can potentially introduce important bugs in the UI I think it's better to leave it for the next team of developers to test and merge it.

@SarahBA SarahBA changed the title Adding UI for publication workflow (the user part) Adding UI for publication workflow Mar 1, 2018
@SarahBA
Copy link
Contributor Author

SarahBA commented Mar 1, 2018 •
edited
Loading

Important notes:

  • Only the first commit was reviewed and approved, not the second commit.
  • Need to be tested by multiple users and community admins. The pull request hasn't been tested enough and can still have bugs.
  • The mockups for workflow can be found in B2share's google drive

TODO:

  • Send an email to the record owner and notify her if the submitted record was accepted or rejected.
  • For communities with 'review and publish' workflow, make sure that the community admin is the only one who can edit 'published' record.

SarahBA
SarahBA commented Mar 1, 2018
View reviewed changes
webui/src/components/user.jsx
communitiesList = communitiesListTemp.reduce(function(map, community){
map[community.get('id').replace(new RegExp("-",'g'),"")] = community.get('name');
return map;
},{});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing #1575

@SarahBA SarahBA mentioned this pull request Mar 1, 2018
Closed
@hjhsalo hjhsalo self-assigned this Jan 28, 2019
@hjhsalo
Copy link
Member

hjhsalo commented Feb 11, 2019

Deployed to test instance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emanueldima emanueldima emanueldima approved these changes

Assignees

@hjhsalo hjhsalo

Labels
In Progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SarahBA @emanueldima @hjhsalo