usnistgov / oar-pdr

The NIST Open Access to Research (OAR) Public Data Repository (PDR) system software
11 stars 10 forks source link

Fix/odd857 new workflow #122

Closed chuanlin2018 closed 4 years ago

chuanlin2018 commented 4 years ago

http://mml.nist.gov:8080/browse/ODD-857

I made three checkins. The latest two were just minor fix and code clean up.

This fix made the following changes:

  1. If edit is enabled, the app will be in edit mode when start up. It will not read nerdm record from transferstate but do authentication and load draft instead. It behaves the same as user clicks on the edit button in previous version.
  2. The edit control buttons are: Preview/Edit, Done and Discard. Preview/Edit button: In edit mode, Preview button will be active. All edit controls in editable fields will be active. In preview mode, Edit button will be active. All edit controls in editable fields will be hidden. Done button: This button will be active if any of the editable fields was modified. When user click on Done button, the app send PATCH to backend to end the edit session. All buttons will be disabled. Discard button: This button will be active if any of the editable fields was modified. When user click on this button, the app send PATCH to backend to reset the data to original state. The app will be set to edit mode.
  3. If user is authenticated but loading draft return error, an "Ooops" message will display with detail info in email body.

This is how I test:

  1. In app.module.ts, enable fakeBackendProvider to bypass the authentication
  2. In environment.ts, set: useMetadataService: false, useCustomizationService: false editEnabled: true
  3. Enable the following code in app/_helpers/fakeBackendInterceptor.ts: // Simulate loading draft if (request.url.indexOf('/customization/api/draft') > -1 && request.method === 'GET') { console.log("Interceptor returning sample record..."); return of(new HttpResponse({ status: 200, body: sampleRecord })); }
  4. Use http://localhost:4200/od/id/test1 for local testing
  5. Use any other ediid to test error handling
  6. In app/_helpers/fakeBackendInterceptor.ts enable "Simulate loading draft error" session to simulate loaddraft error.

Just checked in with following changes:

messagebar.component.html : Added msg.text to the display. messagebar.component.css: Changed warning color from black to dark orange. Auth.service.ts: Remove re-direct parameter "editmode=true" because we don't need it for the new workflow (the app calls startEdit() at very beginning of ngOninit) Customization.service.ts: Updated draftapi value to "pdr/lp/editor/" . editstatus.service.ts: Updated one comment Landingpage.component.ts: always call startEditing() if Editing is enabled.

chuanlin2018 commented 4 years ago

I actually made 4 checkins. The last one was for fake backend. Local makedist ran with no error but Travis generated error "The command "cd docker && bash ./dockbuild.sh" failed and exited with 2 during ." Ray may need to restart it.

chuanlin2018 commented 4 years ago

I fixed the name convention and made editControl component responsible for detecting edit mode. Can you check if that's what you talked about? Based on this change, the editmode in editStatus service doesn't have to be a BehaviorSubject. But since the functionality is the same I didn't change it back.

RayPlante commented 4 years ago

This definitely addresses my concern, and I think the change makes things cleaner and more maintainable. One minor comment: By making changing MetadataUpdateService._setEditMode() to public setEditMode(), you are implying, "Anyone may change the edit mode", which seems counter to your commit comment. You still allow LandingPageComponent to set the mode to VIEWONLY in one instance. This is not a big deal. As a matter of style, if a function is intended for use only by a "friend" class (or two), I prepend the function name with "_" (and don't label it public). The function documentation is a good place to note the intended callers of the function.

chuanlin2018 commented 4 years ago

I see, will fix it. I also noticed one thing that might need be fixed as well.

chuanlin2018 commented 4 years ago

I did further code clean up and add inBrowser condition to startEditing(). Please review again. Thanks.

RayPlante commented 4 years ago

Looks good!

chuanlin2018 commented 4 years ago

I have merged fix/ODD-885-outside-midas into fix/ODD-857-new-workflow.

This was how I tested the change in oar-docker:

After oar-docker started, before I register the test data I tried to open landing page in edit mode. This gave me the page with following text: This record is not currently available for editing. Please return to MIDAS and click "Edit Landing Page" to edit.

After I register the test data, I was able to open the landing page in edit mode.

chuanlin2018 commented 4 years ago

fix/ODD891-author-prop merged into fix/ODD857-new-workflow.

isCollapsed, fnLocked, dataChanged, and originalIndex were filtered from the message send to backend for update.

chuanlin2018 commented 4 years ago

ODD-889 LPS: change affiliation subproperty "dept" to "subunits" - code changes merged into this branch.

RayPlante commented 4 years ago

@chuanlin2018 There's a problem with e2b5072: subunits is being passed as a string, not an array of strings.

chuanlin2018 commented 4 years ago

You are right! Will fix it soon.

chuanlin2018 commented 4 years ago

Just checked in fix/ODD857-new-workflow. The change was to make user double click to select research topics when editing research topics.

RayPlante commented 4 years ago

@chuanlin2018 Did you add a some instructional label in the pop-up that prompts user to double-click?

chuanlin2018 commented 4 years ago

Sorry didn't see your comment until now... Can you recommend the instructional label?

RayPlante commented 4 years ago

Something like, “Double-click on theme to select”.

chuanlin2018 commented 4 years ago

http://mml.nist.gov:8080/browse/ODD-895

In this fix, mouse cursor changes to pointer in edit mode when mouse hovers the editable fields. In preview mode mouse cursor changes to default.

How to test:

In oar-docker publish deployment.yml, set tag name to 'fix/ODD895-topic-looks-like-link' for intpdr.

Deploy, build and start osr-docker and test those editable fields in edit/preview mode.

chuanlin2018 commented 4 years ago

Code checked in for JSON export. Added taxonomyService to the config.

In config server, the following line needs be added to pdr-lps-publish-dev.yml, pdr-lps-publish-local.yml, pdr-lps-publish-test.yml and pdr-lps-publish-prod.yml.

taxonomyService: "https://data.nist.gov/rmm/taxonomy"

To test, run the app in oar-docker using od id 0531A570681DB5D3A1EE2F169DD3B8CE1491. Click on "Export JSON" in the right side menu and "Json" button in metadata page. It should open up the following page:

https://localhost/midas/0531A570681DB5D3A1EE2F169DD3B8CE1491

chuanlin2018 commented 4 years ago

Just checked in the code that added a line of instruction to description popup window. Local testall ran ok but Travis got 3 server errors. Maybe restart it will fix?

chuanlin2018 commented 4 years ago

Just checked in fix/ODD899-Editable Landing Page flashed on load. I tested following 4 cases in local oar-docker:

  1. https://localhost/od/id/0531A570681DB5D3A1EE2F169DD3B8CE1491
  2. https://localhost/od/id/0531A570681DB5D3A1EE2F169DD3B8CE1492
  3. https://localhost/od/id/0531A570681DB5D3A1EE2F169DD3B8CE1491?editEnabled=true
  4. https://localhost/od/id/0531A570681DB5D3A1EE2F169DD3B8CE1492?editEnabled=true

I also set 15sec timeout for the spinner in case some unexpected error occurs and the spinner would spin forever.

deoyani commented 4 years ago

Flashed on load is better. Export JSON and json on metadata page still point to rmm.

chuanlin2018 commented 4 years ago

Just checked in the fix to ODD907 Internal LPS: requests for non-existant/unavailable IDs should return 404. Tested locally in oar-docker.